linux-kernel - Re: [PATCH v6 5/6] fs: prepare for extending 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: <20250701183105.GP10009@frogsfrogsfrogs>
Date: Tue, 1 Jul 2025 11:31:05 -0700
From: "Darrick J. Wong" <djwong@...nel.org>
To: Andrey Albershteyn <aalbersh@...hat.com>
Cc: Amir Goldstein <amir73il@...il.com>, Arnd Bergmann <arnd@...db.de>,
	Casey Schaufler <casey@...aufler-ca.com>,
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	Pali Rohár <pali@...nel.org>,
	Paul Moore <paul@...l-moore.com>, linux-api@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-xfs@...r.kernel.org, selinux@...r.kernel.org,
	Andrey Albershteyn <aalbersh@...nel.org>
Subject: Re: [PATCH v6 5/6] fs: prepare for extending file_get/setattr()

On Mon, Jun 30, 2025 at 06:20:15PM +0200, Andrey Albershteyn wrote:
> From: Amir Goldstein <amir73il@...il.com>
> 
> We intend to add support for more xflags to selective filesystems and
> We cannot rely on copy_struct_from_user() to detect this extension.
> 
> In preparation of extending the API, do not allow setting xflags unknown
> by this kernel version.
> 
> Also do not pass the read-only flags and read-only field fsx_nextents to
> filesystem.
> 
> These changes should not affect existing chattr programs that use the
> ioctl to get fsxattr before setting the new values.
> 
> Link: https://lore.kernel.org/linux-fsdevel/20250216164029.20673-4-pali@kernel.org/
> Cc: Pali Rohár <pali@...nel.org>
> Cc: Andrey Albershteyn <aalbersh@...hat.com>
> Signed-off-by: Amir Goldstein <amir73il@...il.com>
> Signed-off-by: Andrey Albershteyn <aalbersh@...nel.org>
> ---
>  fs/file_attr.c           |  8 +++++++-
>  include/linux/fileattr.h | 20 ++++++++++++++++++++
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/file_attr.c b/fs/file_attr.c
> index 4e85fa00c092..62f08872d4ad 100644
> --- a/fs/file_attr.c
> +++ b/fs/file_attr.c
> @@ -99,9 +99,10 @@ EXPORT_SYMBOL(vfs_fileattr_get);
>  int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
>  {
>  	struct fsxattr xfa;
> +	__u32 mask = FS_XFLAGS_MASK;
>  
>  	memset(&xfa, 0, sizeof(xfa));
> -	xfa.fsx_xflags = fa->fsx_xflags;
> +	xfa.fsx_xflags = fa->fsx_xflags & mask;

I wonder, should it be an error if a filesystem sets an fsx_xflags bit
outside of FS_XFLAGS_MASK?  I guess that's one way to prevent
filesystems from overriding the VFS bits. ;)

Though couldn't that be:

	xfa.fsx_xflags = fa->fsx_xflags & FS_XFLAGS_MASK;

instead?  And same below?

>  	xfa.fsx_extsize = fa->fsx_extsize;
>  	xfa.fsx_nextents = fa->fsx_nextents;
>  	xfa.fsx_projid = fa->fsx_projid;
> @@ -118,11 +119,16 @@ static int copy_fsxattr_from_user(struct fileattr *fa,
>  				  struct fsxattr __user *ufa)
>  {
>  	struct fsxattr xfa;
> +	__u32 mask = FS_XFLAGS_MASK;
>  
>  	if (copy_from_user(&xfa, ufa, sizeof(xfa)))
>  		return -EFAULT;
>  
> +	if (xfa.fsx_xflags & ~mask)
> +		return -EINVAL;

I wonder if you want EOPNOTSUPP here?  We don't know how to support
unknown xflags.  OTOH if you all have beaten this to death while I was
out then don't start another round just for me. :P

--D

> +
>  	fileattr_fill_xflags(fa, xfa.fsx_xflags);
> +	fa->fsx_xflags &= ~FS_XFLAG_RDONLY_MASK;
>  	fa->fsx_extsize = xfa.fsx_extsize;
>  	fa->fsx_nextents = xfa.fsx_nextents;
>  	fa->fsx_projid = xfa.fsx_projid;
> diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
> index 6030d0bf7ad3..e2a2f4ae242d 100644
> --- a/include/linux/fileattr.h
> +++ b/include/linux/fileattr.h
> @@ -14,6 +14,26 @@
>  	 FS_XFLAG_NODUMP | FS_XFLAG_NOATIME | FS_XFLAG_DAX | \
>  	 FS_XFLAG_PROJINHERIT)
>  
> +/* Read-only inode flags */
> +#define FS_XFLAG_RDONLY_MASK \
> +	(FS_XFLAG_PREALLOC | FS_XFLAG_HASATTR)
> +
> +/* Flags to indicate valid value of fsx_ fields */
> +#define FS_XFLAG_VALUES_MASK \
> +	(FS_XFLAG_EXTSIZE | FS_XFLAG_COWEXTSIZE)
> +
> +/* Flags for directories */
> +#define FS_XFLAG_DIRONLY_MASK \
> +	(FS_XFLAG_RTINHERIT | FS_XFLAG_NOSYMLINKS | FS_XFLAG_EXTSZINHERIT)
> +
> +/* Misc settable flags */
> +#define FS_XFLAG_MISC_MASK \
> +	(FS_XFLAG_REALTIME | FS_XFLAG_NODEFRAG | FS_XFLAG_FILESTREAM)
> +
> +#define FS_XFLAGS_MASK \
> +	(FS_XFLAG_COMMON | FS_XFLAG_RDONLY_MASK | FS_XFLAG_VALUES_MASK | \
> +	 FS_XFLAG_DIRONLY_MASK | FS_XFLAG_MISC_MASK)
> +
>  /*
>   * Merged interface for miscellaneous file attributes.  'flags' originates from
>   * ext* and 'fsx_flags' from xfs.  There's some overlap between the two, which
> 
> -- 
> 2.47.2
> 
>