| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <jvo6uj7ro5czlo5ukw3vtf5mpqgrbuksqq4j63s2i6gwrjpz4m@kghpcqyi7gwb>
Date: Mon, 24 Feb 2025 17:00:06 +0100
From: Andrey Albershteyn <aalbersh@...hat.com>
To: Mickaël Salaün <mic@...ikod.net>
Cc: Paul Moore <paul@...l-moore.com>,
Richard Henderson <richard.henderson@...aro.org>, Matt Turner <mattst88@...il.com>,
Russell King <linux@...linux.org.uk>, Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>, Geert Uytterhoeven <geert@...ux-m68k.org>,
Michal Simek <monstr@...str.eu>, Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
"James E.J. Bottomley" <James.Bottomley@...senpartnership.com>, Helge Deller <deller@....de>,
Madhavan Srinivasan <maddy@...ux.ibm.com>, Michael Ellerman <mpe@...erman.id.au>,
Nicholas Piggin <npiggin@...il.com>, Christophe Leroy <christophe.leroy@...roup.eu>,
Naveen N Rao <naveen@...nel.org>, Heiko Carstens <hca@...ux.ibm.com>,
Vasily Gorbik <gor@...ux.ibm.com>, Alexander Gordeev <agordeev@...ux.ibm.com>,
Christian Borntraeger <borntraeger@...ux.ibm.com>, Sven Schnelle <svens@...ux.ibm.com>,
Yoshinori Sato <ysato@...rs.sourceforge.jp>, Rich Felker <dalias@...c.org>,
John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>, "David S. Miller" <davem@...emloft.net>,
Andreas Larsson <andreas@...sler.com>, Andy Lutomirski <luto@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
Chris Zankel <chris@...kel.net>, Max Filippov <jcmvbkbc@...il.com>,
Alexander Viro <viro@...iv.linux.org.uk>, Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
Günther Noack <gnoack@...gle.com>, Arnd Bergmann <arnd@...db.de>, linux-alpha@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-m68k@...ts.linux-m68k.org, linux-mips@...r.kernel.org, linux-parisc@...r.kernel.org,
linuxppc-dev@...ts.ozlabs.org, linux-s390@...r.kernel.org, linux-sh@...r.kernel.org,
sparclinux@...r.kernel.org, linux-fsdevel@...r.kernel.org,
linux-security-module@...r.kernel.org, linux-api@...r.kernel.org, linux-arch@...r.kernel.org,
linux-xfs@...r.kernel.org
Subject: Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
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@...hat.com>
> >
> > 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@...r.kernel.org
> > CC: linux-fsdevel@...r.kernel.org
> > CC: linux-xfs@...r.kernel.org
> > Signed-off-by: Andrey Albershteyn <aalbersh@...hat.com>
> > ---
> > v1:
> > https://lore.kernel.org/linuxppc-dev/20250109174540.893098-1-aalbersh@kernel.org/
> >
> > Previous discussion:
> > https://lore.kernel.org/linux-xfs/20240520164624.665269-2-aalbersh@redhat.com/
> >
> > 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@kernel.org
> > ---
> > 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
Powered by blists - more mailing lists