| Message ID | 20220222175332.384545-1-mic@digikod.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v1] LSM: Remove double path_rename hook calls for RENAME_EXCHANGE | expand |
Any comment? John, Tetsuo, does it look OK for AppArmor and Tomoyo? On 22/02/2022 18:53, Mickaël Salaün wrote: > From: Mickaël Salaün <mic@linux.microsoft.com> > > In order to be able to identify a file exchange with renameat2(2) and > RENAME_EXCHANGE, which will be useful for Landlock [1], propagate the > rename flags to LSMs. This may also improve performance because of the > switch from two set of LSM hook calls to only one, and because LSMs > using this hook may optimize the double check (e.g. only one lock, > reduce the number of path walks). > > AppArmor, Landlock and Tomoyo are updated to leverage this change. This > should not change the current behavior (same check order), except > (different level of) speed boosts. > > [1] https://lore.kernel.org/r/20220221212522.320243-1-mic@digikod.net > > Cc: James Morris <jmorris@namei.org> > Cc: John Johansen <john.johansen@canonical.com> > Cc: Kentaro Takeda <takedakn@nttdata.co.jp> > Cc: Serge E. Hallyn <serge@hallyn.com> > Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com> > Link: https://lore.kernel.org/r/20220222175332.384545-1-mic@digikod.net > --- > include/linux/lsm_hook_defs.h | 2 +- > include/linux/lsm_hooks.h | 1 + > security/apparmor/lsm.c | 30 +++++++++++++++++++++++++----- > security/landlock/fs.c | 12 ++++++++++-- > security/security.c | 9 +-------- > security/tomoyo/tomoyo.c | 11 ++++++++++- > 6 files changed, 48 insertions(+), 17 deletions(-) > > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h > index 819ec92dc2a8..d8b49c9c3a8a 100644 > --- a/include/linux/lsm_hook_defs.h > +++ b/include/linux/lsm_hook_defs.h > @@ -100,7 +100,7 @@ LSM_HOOK(int, 0, path_link, struct dentry *old_dentry, > const struct path *new_dir, struct dentry *new_dentry) > LSM_HOOK(int, 0, path_rename, const struct path *old_dir, > struct dentry *old_dentry, const struct path *new_dir, > - struct dentry *new_dentry) > + struct dentry *new_dentry, unsigned int flags) > LSM_HOOK(int, 0, path_chmod, const struct path *path, umode_t mode) > LSM_HOOK(int, 0, path_chown, const struct path *path, kuid_t uid, kgid_t gid) > LSM_HOOK(int, 0, path_chroot, const struct path *path) > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 3bf5c658bc44..32cd2a7fe9fc 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -358,6 +358,7 @@ > * @old_dentry contains the dentry structure of the old link. > * @new_dir contains the path structure for parent of the new link. > * @new_dentry contains the dentry structure of the new link. > + * @flags may contain rename options such as RENAME_EXCHANGE. > * Return 0 if permission is granted. > * @path_chmod: > * Check for permission to change a mode of the file @path. The new > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c > index 4f0eecb67dde..900bc540656a 100644 > --- a/security/apparmor/lsm.c > +++ b/security/apparmor/lsm.c > @@ -354,13 +354,16 @@ static int apparmor_path_link(struct dentry *old_dentry, const struct path *new_ > } > > static int apparmor_path_rename(const struct path *old_dir, struct dentry *old_dentry, > - const struct path *new_dir, struct dentry *new_dentry) > + const struct path *new_dir, struct dentry *new_dentry, > + const unsigned int flags) > { > struct aa_label *label; > int error = 0; > > if (!path_mediated_fs(old_dentry)) > return 0; > + if ((flags & RENAME_EXCHANGE) && !path_mediated_fs(new_dentry)) > + return 0; > > label = begin_current_label_crit_section(); > if (!unconfined(label)) { > @@ -374,10 +377,27 @@ static int apparmor_path_rename(const struct path *old_dir, struct dentry *old_d > d_backing_inode(old_dentry)->i_mode > }; > > - error = aa_path_perm(OP_RENAME_SRC, label, &old_path, 0, > - MAY_READ | AA_MAY_GETATTR | MAY_WRITE | > - AA_MAY_SETATTR | AA_MAY_DELETE, > - &cond); > + if (flags & RENAME_EXCHANGE) { > + struct path_cond cond_exchange = { > + i_uid_into_mnt(mnt_userns, d_backing_inode(new_dentry)), > + d_backing_inode(new_dentry)->i_mode > + }; > + > + error = aa_path_perm(OP_RENAME_SRC, label, &new_path, 0, > + MAY_READ | AA_MAY_GETATTR | MAY_WRITE | > + AA_MAY_SETATTR | AA_MAY_DELETE, > + &cond_exchange); > + if (!error) > + error = aa_path_perm(OP_RENAME_DEST, label, &old_path, > + 0, MAY_WRITE | AA_MAY_SETATTR | > + AA_MAY_CREATE, &cond_exchange); > + } > + > + if (!error) > + error = aa_path_perm(OP_RENAME_SRC, label, &old_path, 0, > + MAY_READ | AA_MAY_GETATTR | MAY_WRITE | > + AA_MAY_SETATTR | AA_MAY_DELETE, > + &cond); > if (!error) > error = aa_path_perm(OP_RENAME_DEST, label, &new_path, > 0, MAY_WRITE | AA_MAY_SETATTR | > diff --git a/security/landlock/fs.c b/security/landlock/fs.c > index 97b8e421f617..7e57fca6e814 100644 > --- a/security/landlock/fs.c > +++ b/security/landlock/fs.c > @@ -574,10 +574,12 @@ static inline u32 maybe_remove(const struct dentry *const dentry) > static int hook_path_rename(const struct path *const old_dir, > struct dentry *const old_dentry, > const struct path *const new_dir, > - struct dentry *const new_dentry) > + struct dentry *const new_dentry, > + const unsigned int flags) > { > const struct landlock_ruleset *const dom = > landlock_get_current_domain(); > + u32 exchange_access = 0; > > if (!dom) > return 0; > @@ -585,11 +587,17 @@ static int hook_path_rename(const struct path *const old_dir, > if (old_dir->dentry != new_dir->dentry) > /* Gracefully forbids reparenting. */ > return -EXDEV; > + if (flags & RENAME_EXCHANGE) { > + if (unlikely(d_is_negative(new_dentry))) > + return -ENOENT; > + exchange_access = > + get_mode_access(d_backing_inode(new_dentry)->i_mode); > + } > if (unlikely(d_is_negative(old_dentry))) > return -ENOENT; > /* RENAME_EXCHANGE is handled because directories are the same. */ > return check_access_path(dom, old_dir, maybe_remove(old_dentry) | > - maybe_remove(new_dentry) | > + maybe_remove(new_dentry) | exchange_access | > get_mode_access(d_backing_inode(old_dentry)->i_mode)); > } > > diff --git a/security/security.c b/security/security.c > index 22261d79f333..8634da4cfd46 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1184,15 +1184,8 @@ int security_path_rename(const struct path *old_dir, struct dentry *old_dentry, > (d_is_positive(new_dentry) && IS_PRIVATE(d_backing_inode(new_dentry))))) > return 0; > > - if (flags & RENAME_EXCHANGE) { > - int err = call_int_hook(path_rename, 0, new_dir, new_dentry, > - old_dir, old_dentry); > - if (err) > - return err; > - } > - > return call_int_hook(path_rename, 0, old_dir, old_dentry, new_dir, > - new_dentry); > + new_dentry, flags); > } > EXPORT_SYMBOL(security_path_rename); > > diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c > index b6a31901f289..71e82d855ebf 100644 > --- a/security/tomoyo/tomoyo.c > +++ b/security/tomoyo/tomoyo.c > @@ -264,17 +264,26 @@ static int tomoyo_path_link(struct dentry *old_dentry, const struct path *new_di > * @old_dentry: Pointer to "struct dentry". > * @new_parent: Pointer to "struct path". > * @new_dentry: Pointer to "struct dentry". > + * @flags: Rename options. > * > * Returns 0 on success, negative value otherwise. > */ > static int tomoyo_path_rename(const struct path *old_parent, > struct dentry *old_dentry, > const struct path *new_parent, > - struct dentry *new_dentry) > + struct dentry *new_dentry, > + const unsigned int flags) > { > struct path path1 = { .mnt = old_parent->mnt, .dentry = old_dentry }; > struct path path2 = { .mnt = new_parent->mnt, .dentry = new_dentry }; > > + if (flags & RENAME_EXCHANGE) { > + const int err = tomoyo_path2_perm(TOMOYO_TYPE_RENAME, &path2, > + &path1); > + > + if (err) > + return err; > + } > return tomoyo_path2_perm(TOMOYO_TYPE_RENAME, &path1, &path2); > } > > > base-commit: cfb92440ee71adcc2105b0890bb01ac3cddb8507
On 2022/03/23 17:40, Mickaël Salaün wrote: > Any comment? John, Tetsuo, does it look OK for AppArmor and Tomoyo? I'm OK with this change regarding TOMOYO part. Acked-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Thank you.
Looks good to me. Of course now I am going to have to create a follow-up patch to optimize the apparmor mediation. Acked-by: John Johansen <john.johansen@canonical.com> On 3/23/22 01:40, Mickaël Salaün wrote: > Any comment? John, Tetsuo, does it look OK for AppArmor and Tomoyo? > > On 22/02/2022 18:53, Mickaël Salaün wrote: >> From: Mickaël Salaün <mic@linux.microsoft.com> >> >> In order to be able to identify a file exchange with renameat2(2) and >> RENAME_EXCHANGE, which will be useful for Landlock [1], propagate the >> rename flags to LSMs. This may also improve performance because of the >> switch from two set of LSM hook calls to only one, and because LSMs >> using this hook may optimize the double check (e.g. only one lock, >> reduce the number of path walks). >> >> AppArmor, Landlock and Tomoyo are updated to leverage this change. This >> should not change the current behavior (same check order), except >> (different level of) speed boosts. >> >> [1] https://lore.kernel.org/r/20220221212522.320243-1-mic@digikod.net >> >> Cc: James Morris <jmorris@namei.org> >> Cc: John Johansen <john.johansen@canonical.com> >> Cc: Kentaro Takeda <takedakn@nttdata.co.jp> >> Cc: Serge E. Hallyn <serge@hallyn.com> >> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com> >> Link: https://lore.kernel.org/r/20220222175332.384545-1-mic@digikod.net >> --- >> include/linux/lsm_hook_defs.h | 2 +- >> include/linux/lsm_hooks.h | 1 + >> security/apparmor/lsm.c | 30 +++++++++++++++++++++++++----- >> security/landlock/fs.c | 12 ++++++++++-- >> security/security.c | 9 +-------- >> security/tomoyo/tomoyo.c | 11 ++++++++++- >> 6 files changed, 48 insertions(+), 17 deletions(-) >> >> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h >> index 819ec92dc2a8..d8b49c9c3a8a 100644 >> --- a/include/linux/lsm_hook_defs.h >> +++ b/include/linux/lsm_hook_defs.h >> @@ -100,7 +100,7 @@ LSM_HOOK(int, 0, path_link, struct dentry *old_dentry, >> const struct path *new_dir, struct dentry *new_dentry) >> LSM_HOOK(int, 0, path_rename, const struct path *old_dir, >> struct dentry *old_dentry, const struct path *new_dir, >> - struct dentry *new_dentry) >> + struct dentry *new_dentry, unsigned int flags) >> LSM_HOOK(int, 0, path_chmod, const struct path *path, umode_t mode) >> LSM_HOOK(int, 0, path_chown, const struct path *path, kuid_t uid, kgid_t gid) >> LSM_HOOK(int, 0, path_chroot, const struct path *path) >> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >> index 3bf5c658bc44..32cd2a7fe9fc 100644 >> --- a/include/linux/lsm_hooks.h >> +++ b/include/linux/lsm_hooks.h >> @@ -358,6 +358,7 @@ >> * @old_dentry contains the dentry structure of the old link. >> * @new_dir contains the path structure for parent of the new link. >> * @new_dentry contains the dentry structure of the new link. >> + * @flags may contain rename options such as RENAME_EXCHANGE. >> * Return 0 if permission is granted. >> * @path_chmod: >> * Check for permission to change a mode of the file @path. The new >> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c >> index 4f0eecb67dde..900bc540656a 100644 >> --- a/security/apparmor/lsm.c >> +++ b/security/apparmor/lsm.c >> @@ -354,13 +354,16 @@ static int apparmor_path_link(struct dentry *old_dentry, const struct path *new_ >> } >> static int apparmor_path_rename(const struct path *old_dir, struct dentry *old_dentry, >> - const struct path *new_dir, struct dentry *new_dentry) >> + const struct path *new_dir, struct dentry *new_dentry, >> + const unsigned int flags) >> { >> struct aa_label *label; >> int error = 0; >> if (!path_mediated_fs(old_dentry)) >> return 0; >> + if ((flags & RENAME_EXCHANGE) && !path_mediated_fs(new_dentry)) >> + return 0; >> label = begin_current_label_crit_section(); >> if (!unconfined(label)) { >> @@ -374,10 +377,27 @@ static int apparmor_path_rename(const struct path *old_dir, struct dentry *old_d >> d_backing_inode(old_dentry)->i_mode >> }; >> - error = aa_path_perm(OP_RENAME_SRC, label, &old_path, 0, >> - MAY_READ | AA_MAY_GETATTR | MAY_WRITE | >> - AA_MAY_SETATTR | AA_MAY_DELETE, >> - &cond); >> + if (flags & RENAME_EXCHANGE) { >> + struct path_cond cond_exchange = { >> + i_uid_into_mnt(mnt_userns, d_backing_inode(new_dentry)), >> + d_backing_inode(new_dentry)->i_mode >> + }; >> + >> + error = aa_path_perm(OP_RENAME_SRC, label, &new_path, 0, >> + MAY_READ | AA_MAY_GETATTR | MAY_WRITE | >> + AA_MAY_SETATTR | AA_MAY_DELETE, >> + &cond_exchange); >> + if (!error) >> + error = aa_path_perm(OP_RENAME_DEST, label, &old_path, >> + 0, MAY_WRITE | AA_MAY_SETATTR | >> + AA_MAY_CREATE, &cond_exchange); >> + } >> + >> + if (!error) >> + error = aa_path_perm(OP_RENAME_SRC, label, &old_path, 0, >> + MAY_READ | AA_MAY_GETATTR | MAY_WRITE | >> + AA_MAY_SETATTR | AA_MAY_DELETE, >> + &cond); >> if (!error) >> error = aa_path_perm(OP_RENAME_DEST, label, &new_path, >> 0, MAY_WRITE | AA_MAY_SETATTR | >> diff --git a/security/landlock/fs.c b/security/landlock/fs.c >> index 97b8e421f617..7e57fca6e814 100644 >> --- a/security/landlock/fs.c >> +++ b/security/landlock/fs.c >> @@ -574,10 +574,12 @@ static inline u32 maybe_remove(const struct dentry *const dentry) >> static int hook_path_rename(const struct path *const old_dir, >> struct dentry *const old_dentry, >> const struct path *const new_dir, >> - struct dentry *const new_dentry) >> + struct dentry *const new_dentry, >> + const unsigned int flags) >> { >> const struct landlock_ruleset *const dom = >> landlock_get_current_domain(); >> + u32 exchange_access = 0; >> if (!dom) >> return 0; >> @@ -585,11 +587,17 @@ static int hook_path_rename(const struct path *const old_dir, >> if (old_dir->dentry != new_dir->dentry) >> /* Gracefully forbids reparenting. */ >> return -EXDEV; >> + if (flags & RENAME_EXCHANGE) { >> + if (unlikely(d_is_negative(new_dentry))) >> + return -ENOENT; >> + exchange_access = >> + get_mode_access(d_backing_inode(new_dentry)->i_mode); >> + } >> if (unlikely(d_is_negative(old_dentry))) >> return -ENOENT; >> /* RENAME_EXCHANGE is handled because directories are the same. */ >> return check_access_path(dom, old_dir, maybe_remove(old_dentry) | >> - maybe_remove(new_dentry) | >> + maybe_remove(new_dentry) | exchange_access | >> get_mode_access(d_backing_inode(old_dentry)->i_mode)); >> } >> diff --git a/security/security.c b/security/security.c >> index 22261d79f333..8634da4cfd46 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -1184,15 +1184,8 @@ int security_path_rename(const struct path *old_dir, struct dentry *old_dentry, >> (d_is_positive(new_dentry) && IS_PRIVATE(d_backing_inode(new_dentry))))) >> return 0; >> - if (flags & RENAME_EXCHANGE) { >> - int err = call_int_hook(path_rename, 0, new_dir, new_dentry, >> - old_dir, old_dentry); >> - if (err) >> - return err; >> - } >> - >> return call_int_hook(path_rename, 0, old_dir, old_dentry, new_dir, >> - new_dentry); >> + new_dentry, flags); >> } >> EXPORT_SYMBOL(security_path_rename); >> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c >> index b6a31901f289..71e82d855ebf 100644 >> --- a/security/tomoyo/tomoyo.c >> +++ b/security/tomoyo/tomoyo.c >> @@ -264,17 +264,26 @@ static int tomoyo_path_link(struct dentry *old_dentry, const struct path *new_di >> * @old_dentry: Pointer to "struct dentry". >> * @new_parent: Pointer to "struct path". >> * @new_dentry: Pointer to "struct dentry". >> + * @flags: Rename options. >> * >> * Returns 0 on success, negative value otherwise. >> */ >> static int tomoyo_path_rename(const struct path *old_parent, >> struct dentry *old_dentry, >> const struct path *new_parent, >> - struct dentry *new_dentry) >> + struct dentry *new_dentry, >> + const unsigned int flags) >> { >> struct path path1 = { .mnt = old_parent->mnt, .dentry = old_dentry }; >> struct path path2 = { .mnt = new_parent->mnt, .dentry = new_dentry }; >> + if (flags & RENAME_EXCHANGE) { >> + const int err = tomoyo_path2_perm(TOMOYO_TYPE_RENAME, &path2, >> + &path1); >> + >> + if (err) >> + return err; >> + } >> return tomoyo_path2_perm(TOMOYO_TYPE_RENAME, &path1, &path2); >> } >> >> base-commit: cfb92440ee71adcc2105b0890bb01ac3cddb8507
Thanks Tetsuo and John. I'll include this patch in the rename/link Landlock series to fix the remaining issue: https://lore.kernel.org/r/20220221212522.320243-1-mic@digikod.net On 23/03/2022 18:38, John Johansen wrote: > Looks good to me. Of course now I am going to have to create a follow-up > patch to optimize the apparmor mediation. > > Acked-by: John Johansen <john.johansen@canonical.com> > > On 3/23/22 01:40, Mickaël Salaün wrote: >> Any comment? John, Tetsuo, does it look OK for AppArmor and Tomoyo? >> >> On 22/02/2022 18:53, Mickaël Salaün wrote: >>> From: Mickaël Salaün <mic@linux.microsoft.com> >>> >>> In order to be able to identify a file exchange with renameat2(2) and >>> RENAME_EXCHANGE, which will be useful for Landlock [1], propagate the >>> rename flags to LSMs. This may also improve performance because of the >>> switch from two set of LSM hook calls to only one, and because LSMs >>> using this hook may optimize the double check (e.g. only one lock, >>> reduce the number of path walks). >>> >>> AppArmor, Landlock and Tomoyo are updated to leverage this change. This >>> should not change the current behavior (same check order), except >>> (different level of) speed boosts. >>> >>> [1] https://lore.kernel.org/r/20220221212522.320243-1-mic@digikod.net >>> >>> Cc: James Morris <jmorris@namei.org> >>> Cc: John Johansen <john.johansen@canonical.com> >>> Cc: Kentaro Takeda <takedakn@nttdata.co.jp> >>> Cc: Serge E. Hallyn <serge@hallyn.com> >>> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >>> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com> >>> Link: https://lore.kernel.org/r/20220222175332.384545-1-mic@digikod.net >>> --- >>> include/linux/lsm_hook_defs.h | 2 +- >>> include/linux/lsm_hooks.h | 1 + >>> security/apparmor/lsm.c | 30 +++++++++++++++++++++++++----- >>> security/landlock/fs.c | 12 ++++++++++-- >>> security/security.c | 9 +-------- >>> security/tomoyo/tomoyo.c | 11 ++++++++++- >>> 6 files changed, 48 insertions(+), 17 deletions(-) >>> >>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h >>> index 819ec92dc2a8..d8b49c9c3a8a 100644 >>> --- a/include/linux/lsm_hook_defs.h >>> +++ b/include/linux/lsm_hook_defs.h >>> @@ -100,7 +100,7 @@ LSM_HOOK(int, 0, path_link, struct dentry *old_dentry, >>> const struct path *new_dir, struct dentry *new_dentry) >>> LSM_HOOK(int, 0, path_rename, const struct path *old_dir, >>> struct dentry *old_dentry, const struct path *new_dir, >>> - struct dentry *new_dentry) >>> + struct dentry *new_dentry, unsigned int flags) >>> LSM_HOOK(int, 0, path_chmod, const struct path *path, umode_t mode) >>> LSM_HOOK(int, 0, path_chown, const struct path *path, kuid_t uid, kgid_t gid) >>> LSM_HOOK(int, 0, path_chroot, const struct path *path) >>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >>> index 3bf5c658bc44..32cd2a7fe9fc 100644 >>> --- a/include/linux/lsm_hooks.h >>> +++ b/include/linux/lsm_hooks.h >>> @@ -358,6 +358,7 @@ >>> * @old_dentry contains the dentry structure of the old link. >>> * @new_dir contains the path structure for parent of the new link. >>> * @new_dentry contains the dentry structure of the new link. >>> + * @flags may contain rename options such as RENAME_EXCHANGE. >>> * Return 0 if permission is granted. >>> * @path_chmod: >>> * Check for permission to change a mode of the file @path. The new >>> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c >>> index 4f0eecb67dde..900bc540656a 100644 >>> --- a/security/apparmor/lsm.c >>> +++ b/security/apparmor/lsm.c >>> @@ -354,13 +354,16 @@ static int apparmor_path_link(struct dentry *old_dentry, const struct path *new_ >>> } >>> static int apparmor_path_rename(const struct path *old_dir, struct dentry *old_dentry, >>> - const struct path *new_dir, struct dentry *new_dentry) >>> + const struct path *new_dir, struct dentry *new_dentry, >>> + const unsigned int flags) >>> { >>> struct aa_label *label; >>> int error = 0; >>> if (!path_mediated_fs(old_dentry)) >>> return 0; >>> + if ((flags & RENAME_EXCHANGE) && !path_mediated_fs(new_dentry)) >>> + return 0; >>> label = begin_current_label_crit_section(); >>> if (!unconfined(label)) { >>> @@ -374,10 +377,27 @@ static int apparmor_path_rename(const struct path *old_dir, struct dentry *old_d >>> d_backing_inode(old_dentry)->i_mode >>> }; >>> - error = aa_path_perm(OP_RENAME_SRC, label, &old_path, 0, >>> - MAY_READ | AA_MAY_GETATTR | MAY_WRITE | >>> - AA_MAY_SETATTR | AA_MAY_DELETE, >>> - &cond); >>> + if (flags & RENAME_EXCHANGE) { >>> + struct path_cond cond_exchange = { >>> + i_uid_into_mnt(mnt_userns, d_backing_inode(new_dentry)), >>> + d_backing_inode(new_dentry)->i_mode >>> + }; >>> + >>> + error = aa_path_perm(OP_RENAME_SRC, label, &new_path, 0, >>> + MAY_READ | AA_MAY_GETATTR | MAY_WRITE | >>> + AA_MAY_SETATTR | AA_MAY_DELETE, >>> + &cond_exchange); >>> + if (!error) >>> + error = aa_path_perm(OP_RENAME_DEST, label, &old_path, >>> + 0, MAY_WRITE | AA_MAY_SETATTR | >>> + AA_MAY_CREATE, &cond_exchange); >>> + } >>> + >>> + if (!error) >>> + error = aa_path_perm(OP_RENAME_SRC, label, &old_path, 0, >>> + MAY_READ | AA_MAY_GETATTR | MAY_WRITE | >>> + AA_MAY_SETATTR | AA_MAY_DELETE, >>> + &cond); >>> if (!error) >>> error = aa_path_perm(OP_RENAME_DEST, label, &new_path, >>> 0, MAY_WRITE | AA_MAY_SETATTR | >>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c >>> index 97b8e421f617..7e57fca6e814 100644 >>> --- a/security/landlock/fs.c >>> +++ b/security/landlock/fs.c >>> @@ -574,10 +574,12 @@ static inline u32 maybe_remove(const struct dentry *const dentry) >>> static int hook_path_rename(const struct path *const old_dir, >>> struct dentry *const old_dentry, >>> const struct path *const new_dir, >>> - struct dentry *const new_dentry) >>> + struct dentry *const new_dentry, >>> + const unsigned int flags) >>> { >>> const struct landlock_ruleset *const dom = >>> landlock_get_current_domain(); >>> + u32 exchange_access = 0; >>> if (!dom) >>> return 0; >>> @@ -585,11 +587,17 @@ static int hook_path_rename(const struct path *const old_dir, >>> if (old_dir->dentry != new_dir->dentry) >>> /* Gracefully forbids reparenting. */ >>> return -EXDEV; >>> + if (flags & RENAME_EXCHANGE) { >>> + if (unlikely(d_is_negative(new_dentry))) >>> + return -ENOENT; >>> + exchange_access = >>> + get_mode_access(d_backing_inode(new_dentry)->i_mode); >>> + } >>> if (unlikely(d_is_negative(old_dentry))) >>> return -ENOENT; >>> /* RENAME_EXCHANGE is handled because directories are the same. */ >>> return check_access_path(dom, old_dir, maybe_remove(old_dentry) | >>> - maybe_remove(new_dentry) | >>> + maybe_remove(new_dentry) | exchange_access | >>> get_mode_access(d_backing_inode(old_dentry)->i_mode)); >>> } >>> diff --git a/security/security.c b/security/security.c >>> index 22261d79f333..8634da4cfd46 100644 >>> --- a/security/security.c >>> +++ b/security/security.c >>> @@ -1184,15 +1184,8 @@ int security_path_rename(const struct path *old_dir, struct dentry *old_dentry, >>> (d_is_positive(new_dentry) && IS_PRIVATE(d_backing_inode(new_dentry))))) >>> return 0; >>> - if (flags & RENAME_EXCHANGE) { >>> - int err = call_int_hook(path_rename, 0, new_dir, new_dentry, >>> - old_dir, old_dentry); >>> - if (err) >>> - return err; >>> - } >>> - >>> return call_int_hook(path_rename, 0, old_dir, old_dentry, new_dir, >>> - new_dentry); >>> + new_dentry, flags); >>> } >>> EXPORT_SYMBOL(security_path_rename); >>> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c >>> index b6a31901f289..71e82d855ebf 100644 >>> --- a/security/tomoyo/tomoyo.c >>> +++ b/security/tomoyo/tomoyo.c >>> @@ -264,17 +264,26 @@ static int tomoyo_path_link(struct dentry *old_dentry, const struct path *new_di >>> * @old_dentry: Pointer to "struct dentry". >>> * @new_parent: Pointer to "struct path". >>> * @new_dentry: Pointer to "struct dentry". >>> + * @flags: Rename options. >>> * >>> * Returns 0 on success, negative value otherwise. >>> */ >>> static int tomoyo_path_rename(const struct path *old_parent, >>> struct dentry *old_dentry, >>> const struct path *new_parent, >>> - struct dentry *new_dentry) >>> + struct dentry *new_dentry, >>> + const unsigned int flags) >>> { >>> struct path path1 = { .mnt = old_parent->mnt, .dentry = old_dentry }; >>> struct path path2 = { .mnt = new_parent->mnt, .dentry = new_dentry }; >>> + if (flags & RENAME_EXCHANGE) { >>> + const int err = tomoyo_path2_perm(TOMOYO_TYPE_RENAME, &path2, >>> + &path1); >>> + >>> + if (err) >>> + return err; >>> + } >>> return tomoyo_path2_perm(TOMOYO_TYPE_RENAME, &path1, &path2); >>> } >>> >>> base-commit: cfb92440ee71adcc2105b0890bb01ac3cddb8507 >
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index 819ec92dc2a8..d8b49c9c3a8a 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -100,7 +100,7 @@ LSM_HOOK(int, 0, path_link, struct dentry *old_dentry, const struct path *new_dir, struct dentry *new_dentry) LSM_HOOK(int, 0, path_rename, const struct path *old_dir, struct dentry *old_dentry, const struct path *new_dir, - struct dentry *new_dentry) + struct dentry *new_dentry, unsigned int flags) LSM_HOOK(int, 0, path_chmod, const struct path *path, umode_t mode) LSM_HOOK(int, 0, path_chown, const struct path *path, kuid_t uid, kgid_t gid) LSM_HOOK(int, 0, path_chroot, const struct path *path) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 3bf5c658bc44..32cd2a7fe9fc 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -358,6 +358,7 @@ * @old_dentry contains the dentry structure of the old link. * @new_dir contains the path structure for parent of the new link. * @new_dentry contains the dentry structure of the new link. + * @flags may contain rename options such as RENAME_EXCHANGE. * Return 0 if permission is granted. * @path_chmod: * Check for permission to change a mode of the file @path. The new diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 4f0eecb67dde..900bc540656a 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -354,13 +354,16 @@ static int apparmor_path_link(struct dentry *old_dentry, const struct path *new_ } static int apparmor_path_rename(const struct path *old_dir, struct dentry *old_dentry, - const struct path *new_dir, struct dentry *new_dentry) + const struct path *new_dir, struct dentry *new_dentry, + const unsigned int flags) { struct aa_label *label; int error = 0; if (!path_mediated_fs(old_dentry)) return 0; + if ((flags & RENAME_EXCHANGE) && !path_mediated_fs(new_dentry)) + return 0; label = begin_current_label_crit_section(); if (!unconfined(label)) { @@ -374,10 +377,27 @@ static int apparmor_path_rename(const struct path *old_dir, struct dentry *old_d d_backing_inode(old_dentry)->i_mode }; - error = aa_path_perm(OP_RENAME_SRC, label, &old_path, 0, - MAY_READ | AA_MAY_GETATTR | MAY_WRITE | - AA_MAY_SETATTR | AA_MAY_DELETE, - &cond); + if (flags & RENAME_EXCHANGE) { + struct path_cond cond_exchange = { + i_uid_into_mnt(mnt_userns, d_backing_inode(new_dentry)), + d_backing_inode(new_dentry)->i_mode + }; + + error = aa_path_perm(OP_RENAME_SRC, label, &new_path, 0, + MAY_READ | AA_MAY_GETATTR | MAY_WRITE | + AA_MAY_SETATTR | AA_MAY_DELETE, + &cond_exchange); + if (!error) + error = aa_path_perm(OP_RENAME_DEST, label, &old_path, + 0, MAY_WRITE | AA_MAY_SETATTR | + AA_MAY_CREATE, &cond_exchange); + } + + if (!error) + error = aa_path_perm(OP_RENAME_SRC, label, &old_path, 0, + MAY_READ | AA_MAY_GETATTR | MAY_WRITE | + AA_MAY_SETATTR | AA_MAY_DELETE, + &cond); if (!error) error = aa_path_perm(OP_RENAME_DEST, label, &new_path, 0, MAY_WRITE | AA_MAY_SETATTR | diff --git a/security/landlock/fs.c b/security/landlock/fs.c index 97b8e421f617..7e57fca6e814 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -574,10 +574,12 @@ static inline u32 maybe_remove(const struct dentry *const dentry) static int hook_path_rename(const struct path *const old_dir, struct dentry *const old_dentry, const struct path *const new_dir, - struct dentry *const new_dentry) + struct dentry *const new_dentry, + const unsigned int flags) { const struct landlock_ruleset *const dom = landlock_get_current_domain(); + u32 exchange_access = 0; if (!dom) return 0; @@ -585,11 +587,17 @@ static int hook_path_rename(const struct path *const old_dir, if (old_dir->dentry != new_dir->dentry) /* Gracefully forbids reparenting. */ return -EXDEV; + if (flags & RENAME_EXCHANGE) { + if (unlikely(d_is_negative(new_dentry))) + return -ENOENT; + exchange_access = + get_mode_access(d_backing_inode(new_dentry)->i_mode); + } if (unlikely(d_is_negative(old_dentry))) return -ENOENT; /* RENAME_EXCHANGE is handled because directories are the same. */ return check_access_path(dom, old_dir, maybe_remove(old_dentry) | - maybe_remove(new_dentry) | + maybe_remove(new_dentry) | exchange_access | get_mode_access(d_backing_inode(old_dentry)->i_mode)); } diff --git a/security/security.c b/security/security.c index 22261d79f333..8634da4cfd46 100644 --- a/security/security.c +++ b/security/security.c @@ -1184,15 +1184,8 @@ int security_path_rename(const struct path *old_dir, struct dentry *old_dentry, (d_is_positive(new_dentry) && IS_PRIVATE(d_backing_inode(new_dentry))))) return 0; - if (flags & RENAME_EXCHANGE) { - int err = call_int_hook(path_rename, 0, new_dir, new_dentry, - old_dir, old_dentry); - if (err) - return err; - } - return call_int_hook(path_rename, 0, old_dir, old_dentry, new_dir, - new_dentry); + new_dentry, flags); } EXPORT_SYMBOL(security_path_rename); diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c index b6a31901f289..71e82d855ebf 100644 --- a/security/tomoyo/tomoyo.c +++ b/security/tomoyo/tomoyo.c @@ -264,17 +264,26 @@ static int tomoyo_path_link(struct dentry *old_dentry, const struct path *new_di * @old_dentry: Pointer to "struct dentry". * @new_parent: Pointer to "struct path". * @new_dentry: Pointer to "struct dentry". + * @flags: Rename options. * * Returns 0 on success, negative value otherwise. */ static int tomoyo_path_rename(const struct path *old_parent, struct dentry *old_dentry, const struct path *new_parent, - struct dentry *new_dentry) + struct dentry *new_dentry, + const unsigned int flags) { struct path path1 = { .mnt = old_parent->mnt, .dentry = old_dentry }; struct path path2 = { .mnt = new_parent->mnt, .dentry = new_dentry }; + if (flags & RENAME_EXCHANGE) { + const int err = tomoyo_path2_perm(TOMOYO_TYPE_RENAME, &path2, + &path1); + + if (err) + return err; + } return tomoyo_path2_perm(TOMOYO_TYPE_RENAME, &path1, &path2); }