On Fri, Oct 7, 2022 at 5:06 PM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> The current setgid stripping logic during write and ownership change
> operations is inconsistent and strewn over multiple places. In order to
> consolidate it and make more consistent we'll add a new helper
> should_remove_sgid(). The function retains the old behavior where we
> remove the S_ISGID bit unconditionally when S_IXGRP is set but also when
> it isn't set and the caller is neither in the group of the inode nor
> privileged over the inode.
>
> We will use this helper both in write operation permission removal such
> as file_remove_privs() as well as in ownership change operations.
>
> Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
Looks good.
Some suggestions below.
> ---
>
> Notes:
> /* v2 */
> Dave Chinner <dchinner@xxxxxxxxxx>:
> - Use easier to follow logic in the new helper.
>
> fs/attr.c | 27 +++++++++++++++++++++++++++
> fs/internal.h | 2 ++
> 2 files changed, 29 insertions(+)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index b1cff6f5b715..d0bb1dae425e 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -39,6 +39,33 @@ static bool setattr_drop_sgid(struct user_namespace *mnt_userns,
> return true;
> }
>
> +/**
> + * should_remove_sgid - determine whether the setgid bit needs to be removed
> + * @mnt_userns: User namespace of the mount the inode was created from
> + * @inode: inode to check
> + *
> + * This function determines whether the setgid bit needs to be removed.
> + * We retain backwards compatibility and require setgid bit to be removed
> + * unconditionally if S_IXGRP is set. Otherwise we have the exact same
> + * requirements as setattr_prepare() and setattr_copy().
> + *
> + * Return: ATTR_KILL_SGID if setgid bit needs to be removed, 0 otherwise.
> + */
> +int should_remove_sgid(struct user_namespace *mnt_userns,
> + const struct inode *inode)
> +{
> + umode_t mode = inode->i_mode;
> +
> + if (!(mode & S_ISGID))
> + return 0;
> + if (mode & S_IXGRP)
> + return ATTR_KILL_SGID;
> + if (setattr_drop_sgid(mnt_userns, inode,
> + i_gid_into_vfsgid(mnt_userns, inode)))
> + return ATTR_KILL_SGID;
> + return 0;
If you take my suggestion from patch 1/5, that would become:
return setattr_should_remove_sgid(mnt_userns, inode,
i_gid_into_vfsgid(mnt_userns, inode));
> +}
> +
> /**
> * chown_ok - verify permissions to chown inode
> * @mnt_userns: user namespace of the mount @inode was found from
> diff --git a/fs/internal.h b/fs/internal.h
> index 87e96b9024ce..9d165ab65a2a 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -221,3 +221,5 @@ ssize_t do_getxattr(struct user_namespace *mnt_userns,
> int setxattr_copy(const char __user *name, struct xattr_ctx *ctx);
> int do_setxattr(struct user_namespace *mnt_userns, struct dentry *dentry,
> struct xattr_ctx *ctx);
> +int should_remove_sgid(struct user_namespace *mnt_userns,
> + const struct inode *inode);
I realize that you placed this helper in attr.c to make
setattr_drop_sgid() static, but IMO the code will be clearer to readers
if all the family of suig/sgid stripping helpers were clustered together
in inode.c where it will be easier to get the high level view.
Thanks,
Amir.
