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.
+ error = vfs_fileattr_set(file_mnt_idmap(fd_file(dir)),
+ filepath.dentry, &fa);
+ mnt_drop_write(filepath.mnt);
+ }
+
+ path_put(&filepath);
+ return error;
+}
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 638a36be31c14afc66a7fd6eb237d9545e8ad997..dc160c2ef145e4931d625f1f93c2a8ae7f87abf3 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -558,8 +558,7 @@ int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
}
EXPORT_SYMBOL(copy_fsxattr_to_user);
-static int copy_fsxattr_from_user(struct fileattr *fa,
- struct fsxattr __user *ufa)
+int copy_fsxattr_from_user(struct fileattr *fa, struct fsxattr __user *ufa)
{
struct fsxattr xfa;
@@ -646,6 +645,19 @@ static int fileattr_set_prepare(struct inode *inode,
if (fa->fsx_cowextsize == 0)
fa->fsx_xflags &= ~FS_XFLAG_COWEXTSIZE;
+ /*
+ * The only use case for special files is to set project ID, forbid any
+ * other attributes
+ */
+ if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))) {
+ if (fa->fsx_xflags & ~FS_XFLAG_PROJINHERIT)
+ return -EINVAL;
+ if (!S_ISLNK(inode->i_mode) && fa->fsx_nextents)
+ return -EINVAL;
+ if (fa->fsx_extsize || fa->fsx_cowextsize)
+ return -EINVAL;
+ }
+
return 0;
}
diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
[...]
