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

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

on 2022/4/28 10:15, Al Viro wrote:
> On Thu, Apr 28, 2022 at 01:59:01AM +0000, Al Viro wrote:
>> On Tue, Apr 26, 2022 at 07:11:27PM +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.
>>
>> First of all, inode_init_owner() is (and always had been) an optional helper.
>> Filesystems are *NOT* required to call it, so putting any common functionality
>> in there had always been a mistake.
>>
>> That goes for inode_fsuid_set() and inode_fsgid_set() calls as well.
>> Consider e.g. this:
>> struct inode *ext2_new_inode(struct inode *dir, umode_t mode,
>>                               const struct qstr *qstr)
>> {
>> 	...
>>          if (test_opt(sb, GRPID)) {
>> 		inode->i_mode = mode;
>> 		inode->i_uid = current_fsuid();
>> 		inode->i_gid = dir->i_gid;
>> 	} else
>> 		inode_init_owner(&init_user_ns, inode, dir, mode);
>>
>> Here we have an explicit mount option, selecting the way S_ISGID on directories
>> is handled.  Mount ext2 with -o grpid and see for yourself - no inode_init_owner()
>> calls there.
>>
>> The same goes for ext4 - that code is copied there unchanged.
>>
>> What's more, I'm not sure that Jann's fix made any sense in the first place.
>> After all, the file being created here is empty; exec on it won't give you
>> anything - it'll simply fail.  And modifying that file ought to strip SGID,
>> or we have much more interesting problems.
>>
>> What am I missing here?
>
> BTW, xfs has grpid option as well:
> 	if (dir&&  !(dir->i_mode&  S_ISGID)&&  xfs_has_grpid(mp)) {
> 		inode_fsuid_set(inode, mnt_userns);
> 		inode->i_gid = dir->i_gid;
> 		inode->i_mode = mode;
> 	} else {
> 		inode_init_owner(mnt_userns, inode, dir, mode);
> 	}
>
> We could lift that stuff into VFS, but it would require lifting that flag
> (BSD vs. SysV behaviour wrt GID - BSD *always* inherits GID from parent
> and ignores SGID on directories) into generic superblock.  Otherwise we'd
> be breaking existing behaviour for ext* and xfs...

I also mentioned it in my previous version(in the 3/4 patch)
"This patch also changed grpid behaviour for ext4/xfs because the mode 
passed to them may been changed by vfs_prepare_mode.
"

I guess we can add a  grpid option check in vfs_prepare_mode or in 
mode_strip_sgid, then it should not break the existing behaviour for 
ext* and xfs.