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:

quoted

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 <redacted>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---

Reviewed-by: Christian Brauner <redacted>

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