于 2022/4/26 22:52, Jeff Layton 写道:
> On Tue, 2022-04-26 at 19:11 +0800, Yang Xu wrote:
>> Add a dedicated helper to handle the setgid bit when creating a new file
>> in a setgid directory. This is a preparatory patch for moving setgid
>> stripping into the vfs. The patch contains no functional changes.
>>
>> Currently the setgid stripping logic is open-coded directly in
>> inode_init_owner() and the individual filesystems are responsible for
>> handling setgid inheritance. Since this has proven to be brittle as
>> evidenced by old issues we uncovered over the last months (see [1] to
>> [3] below) we will try to move this logic into the vfs.
>>
>> Link: e014f37db1a2 ("xfs: use setattr_copy to set vfs inode attributes") [1]
>> Link: 01ea173e103e ("xfs: fix up non-directory creation in SGID directories") [2]
>> Link: fd84bfdddd16 ("ceph: fix up non-directory creation in SGID directories") [3]
>> Reviewed-by: Darrick J. Wong<djwong@xxxxxxxxxx>
>> Reviewed-by: Christian Brauner (Microsoft)<brauner@xxxxxxxxxx>
>> Signed-off-by: Yang Xu<xuyang2018.jy@xxxxxxxxxxx>
>> ---
>> fs/inode.c | 37 +++++++++++++++++++++++++++++++++----
>> include/linux/fs.h | 2 ++
>> 2 files changed, 35 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/inode.c b/fs/inode.c
>> index 9d9b422504d1..e9a5f2ec2f89 100644
>> --- a/fs/inode.c
>> +++ b/fs/inode.c
>> @@ -2246,10 +2246,8 @@ void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
>> /* Directories are special, and always inherit S_ISGID */
>> if (S_ISDIR(mode))
>> mode |= S_ISGID;
>> - else if ((mode& (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)&&
>> - !in_group_p(i_gid_into_mnt(mnt_userns, dir))&&
>> - !capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
>> - mode&= ~S_ISGID;
>> + else
>> + mode = mode_strip_sgid(mnt_userns, dir, mode);
>> } else
>> inode_fsgid_set(inode, mnt_userns);
>> inode->i_mode = mode;
>> @@ -2405,3 +2403,34 @@ struct timespec64 current_time(struct inode *inode)
>> return timestamp_truncate(now, inode);
>> }
>> EXPORT_SYMBOL(current_time);
>> +
>> +/**
>> + * mode_strip_sgid - handle the sgid bit for non-directories
>> + * @mnt_userns: User namespace of the mount the inode was created from
>> + * @dir: parent directory inode
>> + * @mode: mode of the file to be created in @dir
>> + *
>> + * If the @mode of the new file has both the S_ISGID and S_IXGRP bit
>> + * raised and @dir has the S_ISGID bit raised ensure that the caller is
>> + * either in the group of the parent directory or they have CAP_FSETID
>> + * in their user namespace and are privileged over the parent directory.
>> + * In all other cases, strip the S_ISGID bit from @mode.
>> + *
>> + * Return: the new mode to use for the file
>> + */
>> +umode_t mode_strip_sgid(struct user_namespace *mnt_userns,
>> + const struct inode *dir, umode_t mode)
>> +{
>> + if (S_ISDIR(mode) || !dir || !(dir->i_mode& S_ISGID))
>> + return mode;
>> + if ((mode& (S_ISGID | S_IXGRP)) != (S_ISGID | S_IXGRP))
>> + return mode;
>> + if (in_group_p(i_gid_into_mnt(mnt_userns, dir)))
>> + return mode;
>> + if (capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
>> + return mode;
>> +
>> + mode&= ~S_ISGID;
>> + return mode;
>> +}
>> +EXPORT_SYMBOL(mode_strip_sgid);
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index bbde95387a23..98b44a2732f5 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -1897,6 +1897,8 @@ extern long compat_ptr_ioctl(struct file *file, unsigned int cmd,
>> void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
>> const struct inode *dir, umode_t mode);
>> extern bool may_open_dev(const struct path *path);
>> +umode_t mode_strip_sgid(struct user_namespace *mnt_userns,
>> + const struct inode *dir, umode_t mode);
>>
>> /*
>> * This is the "filldir" function type, used by readdir() to let
>
> This series looks like a nice cleanup. I went ahead and added this pile
> to another kernel I was testing with xfstests and it seemed to do fine.
>
> You can add this (or some variant of it) to all 4 patches.
>
> Reviewed-and-Tested-by: Jeff Layton<jlayton@xxxxxxxxxx>
Thanks, I also have a xfstests patch set[1] for testing this, but now it
is not a final version.
[1]https://patchwork.kernel.org/project/fstests/list/?series=631461
