On Fri, Oct 7, 2022 at 5:06 PM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> In setattr_{copy,prepare}() we need to perform the same permission
> checks to determine whether we need to drop the setgid bit or not.
> Instead of open-coding it twice add a simple helper the encapsulates the
> logic. We will reuse this helpers to make dropping the setgid bit during
> write operations more consistent in a follow up patch.
>
> Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
Looks good.
Some suggestions below - not a must.
Thanks,
Amir.
> ---
>
> Notes:
> /* v2 */
> patch added
>
> fs/attr.c | 29 ++++++++++++++++++++++++-----
> 1 file changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index 1552a5f23d6b..b1cff6f5b715 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -18,6 +18,27 @@
> #include <linux/evm.h>
> #include <linux/ima.h>
>
> +/**
> + * setattr_drop_sgid - check generic setgid permissions
Helper name sounds like a directive, where it should sound like a question.
e.g. setattr_should_remove_sgid()
> + * @mnt_userns: User namespace of the mount the inode was created from
> + * @inode: inode to check
> + * @vfsgid: the new/current vfsgid of @inode
> + *
> + * This function determines whether the setgid bit needs to be removed because
> + * the caller lacks privileges over the inode.
> + *
> + * Return: true if the setgid bit needs to be removed, false if not.
You may want to consider matching the return value to that of
should_remove_sgid(), that is 0 or ATTR_KILL_SGID to make all the family of
those helpers behave similarly.
> + */
> +static bool setattr_drop_sgid(struct user_namespace *mnt_userns,
> + const struct inode *inode, vfsgid_t vfsgid)
> +{
> + if (vfsgid_in_group_p(vfsgid))
> + return false;
> + if (capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID))
> + return false;
> + return true;
> +}
> +
> /**
> * chown_ok - verify permissions to chown inode
> * @mnt_userns: user namespace of the mount @inode was found from
> @@ -140,8 +161,7 @@ int setattr_prepare(struct user_namespace *mnt_userns, struct dentry *dentry,
> vfsgid = i_gid_into_vfsgid(mnt_userns, inode);
>
> /* Also check the setgid bit! */
> - if (!vfsgid_in_group_p(vfsgid) &&
> - !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID))
> + if (setattr_drop_sgid(mnt_userns, inode, vfsgid))
> attr->ia_mode &= ~S_ISGID;
> }
>
> @@ -251,9 +271,8 @@ void setattr_copy(struct user_namespace *mnt_userns, struct inode *inode,
> inode->i_ctime = attr->ia_ctime;
> if (ia_valid & ATTR_MODE) {
> umode_t mode = attr->ia_mode;
> - vfsgid_t vfsgid = i_gid_into_vfsgid(mnt_userns, inode);
> - if (!vfsgid_in_group_p(vfsgid) &&
> - !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID))
> + if (setattr_drop_sgid(mnt_userns, inode,
> + i_gid_into_vfsgid(mnt_userns, inode)))
> mode &= ~S_ISGID;
> inode->i_mode = mode;
> }
> --
> 2.34.1
>
