On Fri, Sep 10, 2021 at 06:57:13PM +0300, Павел Самсонов wrote:
> From 07b6f881080fa18ac404054d43b99433275fe966 Mon Sep 17 00:00:00 2001
> From: Pavel Samsonov <pvsamsonov76@xxxxxxxxx>
> Date: Fri, 10 Sep 2021 14:53:39 +0300
> Subject: [PATCH] Signed-off-by: Pavel Samsonov <pvsamsonov76@xxxxxxxxx>
>
> The patch changes the argument of chown_ok(), chgrp_ok() ...
> functions from *inode to *dentry. The setattr_prepare() function
> has an argument * dentry; it is logical to work with the dentry
> structure in the condition checking functions as well.
Hmmm.
I particularly don't see how this is useful in any way, both chown_ok() and
chgrp_ok() works on the inode itself, not in the dentry. Same goes for
inode_owner_or_capable().
So, you are adding a lot of boiler plate code to each function (and adding a new
extra helper function), just to extract the inode from the dentry on each
helper, instead of just pass the inode directly as how it's ok.
IMHO, just because setattr_prepare() receives a dentry, doesn't mean it needs to
pass the same dentry to the helpers, because, as I said, the helpers itself acts
on the inodes, not in the dentry, so, this patch just adds a lot of unnecessary
code.
But, well, this is my own personal opinion.
Also, for the next time/patches, the patch's subject needs to have the patch
subject, not your SoB.
> ---
> fs/attr.c | 45 +++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index 473d21b3a86d..de1898c19bde 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -21,7 +21,7 @@
> /**
> * chown_ok - verify permissions to chown inode
> * @mnt_userns: user namespace of the mount @inode was found from
> - * @inode: inode to check permissions on
> + * @dentry: dentry to check permissions on
> * @uid: uid to chown @inode to
> *
> * If the inode has been found through an idmapped mount the user namespace
> of
> @@ -31,9 +31,11 @@
> * performed on the raw inode simply passs init_user_ns.
> */
> static bool chown_ok(struct user_namespace *mnt_userns,
> - const struct inode *inode,
> + const struct dentry *dentry,
> kuid_t uid)
> {
> + struct inode *inode = d_inode(dentry);
> +
> kuid_t kuid = i_uid_into_mnt(mnt_userns, inode);
> if (uid_eq(current_fsuid(), kuid) && uid_eq(uid, kuid))
> return true;
> @@ -48,7 +50,7 @@ static bool chown_ok(struct user_namespace *mnt_userns,
> /**
> * chgrp_ok - verify permissions to chgrp inode
> * @mnt_userns: user namespace of the mount @inode was found from
> - * @inode: inode to check permissions on
> + * @dentry: dentry to check permissions on
> * @gid: gid to chown @inode to
> *
> * If the inode has been found through an idmapped mount the user namespace
> of
> @@ -58,8 +60,10 @@ static bool chown_ok(struct user_namespace *mnt_userns,
> * performed on the raw inode simply passs init_user_ns.
> */
> static bool chgrp_ok(struct user_namespace *mnt_userns,
> - const struct inode *inode, kgid_t gid)
> + const struct dentry *dentry, kgid_t gid)
> {
> + struct inode *inode = d_inode(dentry);
> +
> kgid_t kgid = i_gid_into_mnt(mnt_userns, inode);
> if (uid_eq(current_fsuid(), i_uid_into_mnt(mnt_userns, inode)) &&
> (in_group_p(gid) || gid_eq(gid, kgid)))
> @@ -72,6 +76,27 @@ static bool chgrp_ok(struct user_namespace *mnt_userns,
> return false;
> }
>
> +/**
> + * fowner_ok - verify permissions to chmod inode
> + * @mnt_userns: user namespace of the mount @inode was found from
> + * @dentry: dentry to check permissions on
> + *
> + * If the inode has been found through an idmapped mount the user namespace
> of
> + * the vfsmount must be passed through @mnt_userns. This function will then
> + * take care to map the inode according to @mnt_userns before checking
> + * permissions. On non-idmapped mounts or if permission checking is to be
> + * performed on the raw inode simply passs init_user_ns.
> + */
> +static bool fowner_ok(struct user_namespace *mnt_userns,
> + const struct dentry *dentry)
> +{
> + struct inode *inode = d_inode(dentry);
> +
> + if (inode_owner_or_capable(mnt_userns, inode))
> + return true;
> + return false;
> +}
> +
> /**
> * setattr_prepare - check if attribute changes to a dentry are allowed
> * @mnt_userns: user namespace of the mount the inode was found from
> @@ -114,27 +139,31 @@ int setattr_prepare(struct user_namespace *mnt_userns,
> struct dentry *dentry,
> goto kill_priv;
>
> /* Make sure a caller can chown. */
> - if ((ia_valid & ATTR_UID) && !chown_ok(mnt_userns, inode,
> attr->ia_uid))
> + if ((ia_valid & ATTR_UID) && !chown_ok(mnt_userns, dentry,
> attr->ia_uid))
> return -EPERM;
>
> /* Make sure caller can chgrp. */
> - if ((ia_valid & ATTR_GID) && !chgrp_ok(mnt_userns, inode,
> attr->ia_gid))
> + if ((ia_valid & ATTR_GID) && !chgrp_ok(mnt_userns, dentry,
> attr->ia_gid))
> return -EPERM;
>
> /* Make sure a caller can chmod. */
> if (ia_valid & ATTR_MODE) {
> - if (!inode_owner_or_capable(mnt_userns, inode))
> + if (!fowner_ok(mnt_userns, dentry))
> return -EPERM;
> /* Also check the setgid bit! */
> if (!in_group_p((ia_valid & ATTR_GID) ? attr->ia_gid :
> i_gid_into_mnt(mnt_userns, inode)) &&
> !capable_wrt_inode_uidgid(mnt_userns, inode,
> CAP_FSETID))
> attr->ia_mode &= ~S_ISGID;
> + /* Also check the setuid bit! */
> + if (!(capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID) ||
> + uid_eq(current_fsuid(), inode->i_uid)))
> + attr->ia_mode &= ~S_ISUID;
> }
>
> /* Check for setting the inode time. */
> if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET | ATTR_TIMES_SET)) {
> - if (!inode_owner_or_capable(mnt_userns, inode))
> + if (!fowner_ok(mnt_userns, dentry))
> return -EPERM;
> }
>
> --
> 2.30.2
>
>
--
Carlos
