On Thu, May 19, 2022 at 01:03:12AM +0000, xuyang2018.jy@xxxxxxxxxxx wrote: > on 2022/4/28 17:34, Christian Brauner wrote: > > On Thu, Apr 28, 2022 at 03:46:59AM +0000, Al Viro wrote: > >> On Thu, Apr 28, 2022 at 03:12:30AM +0000, Al Viro wrote: > >> > >>>> Note, BTW, that while XFS has inode_fsuid_set() on the non-inode_init_owner() > >>>> path, it doesn't have inode_fsgid_set() there. Same goes for ext4, while > >>>> ext2 doesn't bother with either in such case... > >>>> > >>>> Let's try to separate the issues here. Jann, could you explain what makes > >>>> empty sgid files dangerous? > >>> > >>> Found the original thread in old mailbox, and the method of avoiding the > >>> SGID removal on modification is usable. Which answers the question above... > >> > >> OK, what do we want for grpid mounts? Aside of "don't forget inode_fsuid_set(), > >> please", that is. We don't want inode_fsgid_set() there (whatever went for > >> the parent directory should be the right value for the child). Same "strip > > > > Exactly. You sounded puzzled why we don't call that in an earlier mail. > > > >> SGID from non-directory child, unless we are in the resulting group"? > > > > Honestly, I think we should try and see if we can't use the same setgid > > inheritance enforcement of the new mode_strip_sgid() helper for the > > grpid mount option as well. Iow, just don't give the grpid mount option > > a separate setgid treatment and try it with the current approach. > > > > It'll allow us to move things into vfs proper which I think is a robust > > solution with clear semantics. It also gives us a uniform ordering wrt > > to umask stripping and POSIX ACLs. > > > > Yes, as we've pointed out in the thread this carries a non-zero > > regression risk. But so does the whole patch series. But this might end > > up being a big win security wise and makes maintenance way easier going > > forward. > > > > The current setgid situation is thoroughly messy though and we keep > > plugging holes. Even writing tests for the current situation is an > > almost herculean task let alone reviewing it. > > Sorry for the late reply. > I am agree with these. So what should I do in next step? I'd say we try to go forward with the original approach and not special-casing the grpid option. > > ps:I also think I may send test case to xfstests for posix acl and umask > ASAP, then xfstests can merge these test and so more people will notice > this problem. The big refactoring of the idmapped mounts testsuite into a generic vfstest refactoring is sitting in for-next of xfstests so make sure to base your patches on that because you really won't enjoy having to rebase them...
