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

[Al Viro <viro@zeniv.linux.org.uk>]

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?