linux-kernel - Re: [PATCH v5 0/7] fs: introduce file_getattr and file

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOQ4uxgOAxg7N1OUJfb1KMp7oWOfN=KV9Lzz6ZrX0=XRGOQrEQ@mail.gmail.com>
Date: Tue, 13 May 2025 14:53:17 +0200
From: Amir Goldstein <amir73il@...il.com>
To: Arnd Bergmann <arnd@...db.de>, Christian Brauner <brauner@...nel.org>
Cc: Andrey Albershteyn <aalbersh@...hat.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>, Jan Kara <jack@...e.cz>, 
	Mickaël Salaün <mic@...ikod.net>, 
	Günther Noack <gnoack@...gle.com>, 
	Pali Rohár <pali@...nel.org>, 
	Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>, 
	"Serge E. Hallyn" <serge@...lyn.com>, Stephen Smalley <stephen.smalley.work@...il.com>, 
	Ondrej Mosnacek <omosnace@...hat.com>, Tyler Hicks <code@...icks.com>, 
	Miklos Szeredi <miklos@...redi.hu>, 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 <linux-arch@...r.kernel.org>, selinux@...r.kernel.org, 
	ecryptfs@...r.kernel.org, linux-unionfs@...r.kernel.org, 
	linux-xfs@...r.kernel.org, Andrey Albershteyn <aalbersh@...nel.org>
Subject: Re: [PATCH v5 0/7] fs: introduce file_getattr and file_setattr syscalls

On Tue, May 13, 2025 at 11:53 AM Arnd Bergmann <arnd@...db.de> wrote:
>
> On Tue, May 13, 2025, at 11:17, Andrey Albershteyn wrote:
>
> >
> >       long syscall(SYS_file_getattr, int dirfd, const char *pathname,
> >               struct fsxattr *fsx, size_t size, unsigned int at_flags);
> >       long syscall(SYS_file_setattr, int dirfd, const char *pathname,
> >               struct fsxattr *fsx, size_t size, unsigned int at_flags);
>
> I don't think we can have both the "struct fsxattr" from the uapi
> headers, and a variable size as an additional argument. I would
> still prefer not having the extensible structure at all and just
> use fsxattr, but if you want to make it extensible in this way,
> it should use a different structure (name). Otherwise adding
> fields after fsx_pad[] would break the ioctl interface.
>

Are you are suggesting that we need to define this variant?:

--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -148,6 +148,17 @@ struct fsxattr {
        unsigned char   fsx_pad[8];
 };

+/*
+ * Variable size structure for file_[sg]et_attr().
+ */
+struct fsx_fileattr {
+       __u32           fsx_xflags;     /* xflags field value (get/set) */
+       __u32           fsx_extsize;    /* extsize field value (get/set)*/
+       __u32           fsx_nextents;   /* nextents field value (get)   */
+       __u32           fsx_projid;     /* project identifier (get/set) */
+       __u32           fsx_cowextsize; /* CoW extsize field value (get/set)*/
+};
+

> I also find the bit confusing where the argument contains both
> "ignored but assumed zero" flags, and "required to be zero"
> flags depending on whether it's in the fsx_pad[] field or
> after it. This would be fine if it was better documented.
>

I think that is an oversight.
The syscall should have required that fsx_pad is zero,
same as patch 6/7 requires that unknown xflags are zero.

If we change to:
       error = copy_struct_from_user(&fsx, sizeof(struct
fsx_fileattr), ufsx, usize);

It will take care of requiring zero fsx_pad even if user calls the syscall with
sizeof(struct fsxattr).

Thanks,
Amir.