| Message ID | 0189aeb1-75f1-1e8b-71a8-ea6a7641518b@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Signed-off-by: Pavel Samsonov <pvsamsonov76@gmail.com> | expand |
On Fri, Sep 10, 2021 at 06:57:13PM +0300, Павел Самсонов wrote: > From 07b6f881080fa18ac404054d43b99433275fe966 Mon Sep 17 00:00:00 2001 > From: Pavel Samsonov <pvsamsonov76@gmail.com> > Date: Fri, 10 Sep 2021 14:53:39 +0300 > Subject: [PATCH] Signed-off-by: Pavel Samsonov <pvsamsonov76@gmail.com> > > 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 > >
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
From 07b6f881080fa18ac404054d43b99433275fe966 Mon Sep 17 00:00:00 2001 From: Pavel Samsonov <pvsamsonov76@gmail.com> Date: Fri, 10 Sep 2021 14:53:39 +0300 Subject: [PATCH] Signed-off-by: Pavel Samsonov <pvsamsonov76@gmail.com> 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. --- fs/attr.c | 45 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 8 deletions(-) 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; }