On Wed, Oct 05, 2022 at 05:14:31PM +0200, Christian Brauner wrote:
> diff --git a/fs/inode.c b/fs/inode.c
> index ba1de23c13c1..4f3257f5ed7a 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1949,26 +1949,44 @@ void touch_atime(const struct path *path)
> }
> EXPORT_SYMBOL(touch_atime);
>
> -/*
> - * The logic we want is
> +/**
> + * 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 where we require the setgid bit to be
> + * removed unconditionally if S_IXGRP is set. Otherwise we have the exact same
> + * requirements as setattr_prepare() and setattr_copy().
> *
> - * if suid or (sgid and xgrp)
> - * remove privs
> + * Return: true if setgit bit needs to be removed, false otherwise.
> */
> -int should_remove_suid(struct dentry *dentry)
> +static bool should_remove_sgid(struct user_namespace *mnt_userns,
> + struct inode *inode)
> +{
> + umode_t mode = inode->i_mode;
> +
> + if (unlikely(mode & S_ISGID)) {
> + if ((mode & S_IXGRP) ||
> + (!vfsgid_in_group_p(i_gid_into_vfsgid(mnt_userns, inode)) &&
> + !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID)))
> + return true;
> + }
> +
> + return false;
I find this sort of convoluted logic much easier to follow when it's
written as a stacked set of single comparisons like so:
if (!(mode & S_ISGID))
return false;
if (mode & S_IXGRP)
return true;
if (vfsgid_in_group_p(i_gid_into_vfsgid(mnt_userns, inode))
return false;
if (capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID)
return false;
return true;
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
