Re: [PATCH v8 1/4] fs: add mode_strip_sgid() helper

["xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com>]

于 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@kernel.org>
>> Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   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@kernel.org>

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