on 2022/4/19 22:09, Christian Brauner wrote:
> On Tue, Apr 19, 2022 at 07:47:13PM +0800, Yang Xu wrote:
>> Currently, vfs only passes mode argument to filesystem, then use inode_init_owner()
>> to strip S_ISGID. Some filesystem(ie ext4/btrfs) will call inode_init_owner
>> firstly, then posxi acl setup, but xfs uses the contrary order. It will affect
>> S_ISGID clear especially we filter S_IXGRP by umask or acl.
>>
>> Regardless of which filesystem is in use, failure to strip the SGID correctly is
>> considered a security failure that needs to be fixed. The current VFS infrastructure
>> requires the filesystem to do everything right and not step on any landmines to
>> strip the SGID bit, when in fact it can easily be done at the VFS and the filesystems
>> then don't even need to be aware that the SGID needs to be (or has been stripped) by
>> the operation the user asked to be done.
>>
>> Vfs has all the info it needs - it doesn't need the filesystems to do everything
>> correctly with the mode and ensuring that they order things like posix acl setup
>> functions correctly with inode_init_owner() to strip the SGID bit.
>>
>> Just strip the SGID bit at the VFS, and then the filesystems can't get it wrong.
>>
>> Also, the inode_sgid_strip() api should be used before IS_POSIXACL() because
>> this api may change mode.
>>
>> Only the following places use inode_init_owner
>> "
>> arch/powerpc/platforms/cell/spufs/inode.c: inode_init_owner(&init_user_ns, inode, dir, mode | S_IFDIR);
>> arch/powerpc/platforms/cell/spufs/inode.c: inode_init_owner(&init_user_ns, inode, dir, mode | S_IFDIR);
>> fs/9p/vfs_inode.c: inode_init_owner(&init_user_ns, inode, NULL, mode);
>> fs/bfs/dir.c: inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/btrfs/inode.c: inode_init_owner(mnt_userns, inode, dir, mode);
>> fs/btrfs/tests/btrfs-tests.c: inode_init_owner(&init_user_ns, inode, NULL, S_IFREG);
>> fs/ext2/ialloc.c: inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/ext4/ialloc.c: inode_init_owner(mnt_userns, inode, dir, mode);
>> fs/f2fs/namei.c: inode_init_owner(mnt_userns, inode, dir, mode);
>> fs/hfsplus/inode.c: inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/hugetlbfs/inode.c: inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/jfs/jfs_inode.c: inode_init_owner(&init_user_ns, inode, parent, mode);
>> fs/minix/bitmap.c: inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/nilfs2/inode.c: inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/ntfs3/inode.c: inode_init_owner(mnt_userns, inode, dir, mode);
>> fs/ocfs2/dlmfs/dlmfs.c: inode_init_owner(&init_user_ns, inode, NULL, mode);
>> fs/ocfs2/dlmfs/dlmfs.c: inode_init_owner(&init_user_ns, inode, parent, mode);
>> fs/ocfs2/namei.c: inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/omfs/inode.c: inode_init_owner(&init_user_ns, inode, NULL, mode);
>> fs/overlayfs/dir.c: inode_init_owner(&init_user_ns, inode, dentry->d_parent->d_inode, mode);
>> fs/ramfs/inode.c: inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/reiserfs/namei.c: inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/sysv/ialloc.c: inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/ubifs/dir.c: inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/udf/ialloc.c: inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/ufs/ialloc.c: inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/xfs/xfs_inode.c: inode_init_owner(mnt_userns, inode, dir, mode);
>> fs/zonefs/super.c: inode_init_owner(&init_user_ns, inode, parent, S_IFDIR | 0555);
>> kernel/bpf/inode.c: inode_init_owner(&init_user_ns, inode, dir, mode);
>> mm/shmem.c: inode_init_owner(&init_user_ns, inode, dir, mode);
>> "
>>
>> They are used in filesystem init new inode function and these init inode functions are used
>> by following operations:
>> mkdir
>> symlink
>> mknod
>> create
>> tmpfile
>> rename
>>
>> We don't care about mkdir because we don't strip SGID bit for directory except fs.xfs.irix_sgid_inherit.
>> But we even call it in do_mkdirat() since inode_sgid_strip() will skip directories anyway. This will
>> enforce the same ordering for all relevant operations and it will make the code more uniform and
>> easier to understand by using new helper prepare_mode().
>>
>> symlink and rename only use valid mode that doesn't have SGID bit.
>>
>> We have added inode_sgid_strip api for the remaining operations.
>>
>> In addition to the above six operations, four filesystems has a little difference
>> 1) btrfs has btrfs_create_subvol_root to create new inode but used non SGID bit mode and can ignore
>> 2) ocfs2 reflink function should add inode_sgid_strip api manually because we don't add it in vfs
>> 3) spufs which doesn't really go hrough the regular VFS callpath because it has separate system call
>> spu_create, but it t only allows the creation of directories and only allows bits in 0777 and can ignore
>> 4)bpf use vfs_mkobj in bpf_obj_do_pin with "S_IFREG | ((S_IRUSR | S_IWUSR)& ~current_umask()) mode and
>> use bpf_mkobj_ops in bpf_iter_link_pin_kernel with S_IFREG | S_IRUSR; , so bpf is also not affected
>>
>> This patch also changed grpid behaviour for ext4/xfs because the mode passed to them may been
>> changed by inode_sgid_strip.
>>
>> Also as Christian Brauner said"
>> The patch itself is useful as it would move a security sensitive operation that is currently burried in
>> individual filesystems into the vfs layer. But it has a decent regression potential since it might strip
>> filesystems that have so far relied on getting the S_ISGID bit with a mode argument. So this needs a lot
>> of testing and long exposure in -next for at least one full kernel cycle."
>>
>> Suggested-by: Dave Chinner<[email protected]>
>> Signed-off-by: Yang Xu<[email protected]>
>> ---
>
> I think we're getting closer but please focus the patch series. This has
> morphed into an 8 patch series where 4 or 5 of these patches are fixes
> that a) I'm not sure are worth it or fix anything b) they are filesystem
> specific and can be independently upstreamed and c) have nothing to do
> with the core of this patch series.
>
> So I'd suggest you'd just make this about sgid stripping and then this
> doesn't have to be more than 3 maybe 4 patches, imho.
Ok, will focus on this sgid stripping.