On 2025-02-21 16:08:33, Mickaël Salaün wrote: > It looks security checks are missing. With IOCTL commands, file > permissions are checked at open time, but with these syscalls the path > is only resolved but no specific access seems to be checked (except > inode_owner_or_capable via vfs_fileattr_set). > > On Tue, Feb 11, 2025 at 06:22:47PM +0100, Andrey Albershteyn wrote: > > From: Andrey Albershteyn <aalbersh@xxxxxxxxxx> > > > > Introduce getfsxattrat and setfsxattrat syscalls to manipulate inode > > extended attributes/flags. The syscalls take parent directory fd and > > path to the child together with struct fsxattr. > > > > This is an alternative to FS_IOC_FSSETXATTR ioctl with a difference > > that file don't need to be open as we can reference it with a path > > instead of fd. By having this we can manipulated inode extended > > attributes not only on regular files but also on special ones. This > > is not possible with FS_IOC_FSSETXATTR ioctl as with special files > > we can not call ioctl() directly on the filesystem inode using fd. > > > > This patch adds two new syscalls which allows userspace to get/set > > extended inode attributes on special files by using parent directory > > and a path - *at() like syscall. > > > > Also, as vfs_fileattr_set() is now will be called on special files > > too, let's forbid any other attributes except projid and nextents > > (symlink can have an extent). > > > > CC: linux-api@xxxxxxxxxxxxxxx > > CC: linux-fsdevel@xxxxxxxxxxxxxxx > > CC: linux-xfs@xxxxxxxxxxxxxxx > > Signed-off-by: Andrey Albershteyn <aalbersh@xxxxxxxxxx> > > --- > > v1: > > https://lore.kernel.org/linuxppc-dev/20250109174540.893098-1-aalbersh@xxxxxxxxxx/ > > > > Previous discussion: > > https://lore.kernel.org/linux-xfs/20240520164624.665269-2-aalbersh@xxxxxxxxxx/ > > > > XFS has project quotas which could be attached to a directory. All > > new inodes in these directories inherit project ID set on parent > > directory. > > > > The project is created from userspace by opening and calling > > FS_IOC_FSSETXATTR on each inode. This is not possible for special > > files such as FIFO, SOCK, BLK etc. Therefore, some inodes are left > > with empty project ID. Those inodes then are not shown in the quota > > accounting but still exist in the directory. Moreover, in the case > > when special files are created in the directory with already > > existing project quota, these inode inherit extended attributes. > > This than leaves them with these attributes without the possibility > > to clear them out. This, in turn, prevents userspace from > > re-creating quota project on these existing files. > > --- > > Changes in v3: > > - Remove unnecessary "dfd is dir" check as it checked in user_path_at() > > - Remove unnecessary "same filesystem" check > > - Use CLASS() instead of directly calling fdget/fdput > > - Link to v2: https://lore.kernel.org/r/20250122-xattrat-syscall-v2-1-5b360d4fbcb2@xxxxxxxxxx > > --- > > arch/alpha/kernel/syscalls/syscall.tbl | 2 + > > arch/arm/tools/syscall.tbl | 2 + > > arch/arm64/tools/syscall_32.tbl | 2 + > > arch/m68k/kernel/syscalls/syscall.tbl | 2 + > > arch/microblaze/kernel/syscalls/syscall.tbl | 2 + > > arch/mips/kernel/syscalls/syscall_n32.tbl | 2 + > > arch/mips/kernel/syscalls/syscall_n64.tbl | 2 + > > arch/mips/kernel/syscalls/syscall_o32.tbl | 2 + > > arch/parisc/kernel/syscalls/syscall.tbl | 2 + > > arch/powerpc/kernel/syscalls/syscall.tbl | 2 + > > arch/s390/kernel/syscalls/syscall.tbl | 2 + > > arch/sh/kernel/syscalls/syscall.tbl | 2 + > > arch/sparc/kernel/syscalls/syscall.tbl | 2 + > > arch/x86/entry/syscalls/syscall_32.tbl | 2 + > > arch/x86/entry/syscalls/syscall_64.tbl | 2 + > > arch/xtensa/kernel/syscalls/syscall.tbl | 2 + > > fs/inode.c | 75 +++++++++++++++++++++++++++++ > > fs/ioctl.c | 16 +++++- > > include/linux/fileattr.h | 1 + > > include/linux/syscalls.h | 4 ++ > > include/uapi/asm-generic/unistd.h | 8 ++- > > 21 files changed, 133 insertions(+), 3 deletions(-) > > > > [...] > > > diff --git a/fs/inode.c b/fs/inode.c > > index 6b4c77268fc0ecace4ac78a9ca777fbffc277f4a..b2dddd9db4fabaf67a6cbf541a86978b290411ec 100644 > > --- a/fs/inode.c > > +++ b/fs/inode.c > > @@ -23,6 +23,9 @@ > > #include <linux/rw_hint.h> > > #include <linux/seq_file.h> > > #include <linux/debugfs.h> > > +#include <linux/syscalls.h> > > +#include <linux/fileattr.h> > > +#include <linux/namei.h> > > #include <trace/events/writeback.h> > > #define CREATE_TRACE_POINTS > > #include <trace/events/timestamp.h> > > @@ -2953,3 +2956,75 @@ umode_t mode_strip_sgid(struct mnt_idmap *idmap, > > return mode & ~S_ISGID; > > } > > EXPORT_SYMBOL(mode_strip_sgid); > > + > > +SYSCALL_DEFINE4(getfsxattrat, int, dfd, const char __user *, filename, > > + struct fsxattr __user *, fsx, unsigned int, at_flags) > > +{ > > + CLASS(fd, dir)(dfd); > > + struct fileattr fa; > > + struct path filepath; > > + int error; > > + unsigned int lookup_flags = 0; > > + > > + if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0) > > + return -EINVAL; > > + > > + if (at_flags & AT_SYMLINK_FOLLOW) > > + lookup_flags |= LOOKUP_FOLLOW; > > + > > + if (at_flags & AT_EMPTY_PATH) > > + lookup_flags |= LOOKUP_EMPTY; > > + > > + if (fd_empty(dir)) > > + return -EBADF; > > + > > + error = user_path_at(dfd, filename, lookup_flags, &filepath); > > + if (error) > > + return error; > > security_inode_getattr() should probably be called here. > > > + > > + error = vfs_fileattr_get(filepath.dentry, &fa); > > + if (!error) > > + error = copy_fsxattr_to_user(&fa, fsx); > > + > > + path_put(&filepath); > > + return error; > > +} > > + > > +SYSCALL_DEFINE4(setfsxattrat, int, dfd, const char __user *, filename, > > + struct fsxattr __user *, fsx, unsigned int, at_flags) > > +{ > > + CLASS(fd, dir)(dfd); > > + struct fileattr fa; > > + struct path filepath; > > + int error; > > + unsigned int lookup_flags = 0; > > + > > + if ((at_flags & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0) > > + return -EINVAL; > > + > > + if (at_flags & AT_SYMLINK_FOLLOW) > > + lookup_flags |= LOOKUP_FOLLOW; > > + > > + if (at_flags & AT_EMPTY_PATH) > > + lookup_flags |= LOOKUP_EMPTY; > > + > > + if (fd_empty(dir)) > > + return -EBADF; > > + > > + if (copy_fsxattr_from_user(&fa, fsx)) > > + return -EFAULT; > > + > > + error = user_path_at(dfd, filename, lookup_flags, &filepath); > > + if (error) > > + return error; > > + > > + error = mnt_want_write(filepath.mnt); > > + if (!error) { > > security_inode_setattr() should probably be called too. Aren't those checks for something different - inode attributes ATTR_*? (sorry, the naming can't be more confusing) Looking into security_inode_setattr() it seems to expect struct iattr, which works with inode attributes (mode, time, uid/gid...). These new syscalls work with filesystem inode extended flags/attributes FS_XFLAG_* in fsxattr->fsx_xflags. Let me know if I missing something here -- - Andrey
