On Thu, Jan 14, 2021 at 11:45:11AM +0100, Christian Brauner wrote:
> On Wed, Jan 13, 2021 at 07:46:30PM +0100, Christoph Hellwig wrote:
> > XFS always inherits the SGID bit if it is set on the parent inode, while
> > the generic inode_init_owner does not do this in a few cases where it can
> > create a possible security problem, see commit 0fa3ecd87848
> > ("Fix up non-directory creation in SGID directories") for details.
> >
> > Switch XFS to use the generic helper for the normal path to fix this,
> > just keeping the simple field inheritance open coded for the case of the
> > non-sgid case with the bsdgrpid mount option.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Reported-by: Christian Brauner <christian.brauner@xxxxxxxxxx>
> > Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> > ---
>
> Reviewed-by: Christian Brauner <christian.brauner@xxxxxxxxxx>
>
> I ran the idmapped mounts xfstests on this patchset. With this patch
> applied I was able to remove the special casing for xfs (apart from the
> irix compatibility check) and got clean test runs:
>
>
> 1. with regular setgid inheritance rules
> root@f2-vm:/xfstests# ./check generic/622
Is this test posted to fstests somewhere?
FWIW the code change looks reasonable to me, but I wanted to see the
functionality exercise test first. :)
--D
> FSTYP -- xfs (non-debug)
> PLATFORM -- Linux/x86_64 f2-vm 5.11.0-rc3-brauner-idmapped-mounts-xfs #311 SMP Thu Jan 14 09:55:14 UTC 2021
> MKFS_OPTIONS -- -f -bsize=4096 /dev/loop7
> MOUNT_OPTIONS -- /dev/loop7 /mnt/scratch
>
> generic/622 1s ... 2s
> Ran: generic/622
> Passed all 1 tests
>
> 2. with irix_sgid_inherit setgid inheritance rules
> root@f2-vm:/xfstests# echo 1 > /proc/sys/fs/xfs/irix_sgid_inherit
> root@f2-vm:/xfstests# ./check generic/622
> FSTYP -- xfs (non-debug)
> PLATFORM -- Linux/x86_64 f2-vm 5.11.0-rc3-brauner-idmapped-mounts-xfs #311 SMP Thu Jan 14 09:55:14 UTC 2021
> MKFS_OPTIONS -- -f -bsize=4096 /dev/loop7
> MOUNT_OPTIONS -- /dev/loop7 /mnt/scratch
>
> generic/622 2s ... 1s
> Ran: generic/622
> Passed all 1 tests
>
> Thanks!
> Christian
