linux-kernel - Re: [PATCH] fs: introduce getfsxattrat and setfsxattrat syscalls

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: <doha6zamxgmqapwx4r6ehzbatzar4dcep33zehunonqforjzf5@lxpidn37tdjh>
Date: Mon, 13 Jan 2025 12:19:36 +0100
From: Jan Kara <jack@...e.cz>
To: Andrey Albershteyn <aalbersh@...hat.com>
Cc: linux-fsdevel@...r.kernel.org, linux-api@...r.kernel.org, 
	monstr@...str.eu, mpe@...erman.id.au, npiggin@...il.com, 
	christophe.leroy@...roup.eu, naveen@...nel.org, maddy@...ux.ibm.com, luto@...nel.org, 
	tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com, 
	x86@...nel.org, hpa@...or.com, chris@...kel.net, jcmvbkbc@...il.com, 
	viro@...iv.linux.org.uk, brauner@...nel.org, jack@...e.cz, arnd@...db.de, 
	linux-alpha@...r.kernel.org, linux-kernel@...r.kernel.org, linux-m68k@...ts.linux-m68k.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-security-module@...r.kernel.org, linux-arch@...r.kernel.org
Subject: Re: [PATCH] fs: introduce getfsxattrat and setfsxattrat syscalls

On Thu 09-01-25 18:45:40, 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. By having this we can manipulated
> inode extended attributes not only on normal files but also on
> special ones. This is not possible with FS_IOC_FSSETXATTR ioctl as
> opening special files returns VFS special inode instead of
> underlying filesystem one.
> 
> This patch adds two new syscalls which allows userspace to set
> extended inode attributes on special files by using parent directory
> to open FS inode.
> 
> 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
> Signed-off-by: Andrey Albershteyn <aalbersh@...hat.com>

Couple of comments below:

> @@ -2953,3 +2956,105 @@ 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 *, fsx, int, at_flags)
				       ^^^ at_flags should be probably
unsigned - at least they seem to be for other syscalls.

> +{
> +	struct fd dir;
> +	struct fileattr fa;
> +	struct path filepath;
> +	struct inode *inode;
> +	int error;
> +
> +	if (at_flags)
> +		return -EINVAL;

Shouldn't we support basic path resolve flags like AT_SYMLINK_NOFOLLOW or
AT_EMPTY_PATH? I didn't put too much thought to this but intuitively I'd say
we should follow what path_setxattrat() does.

> +
> +	if (!capable(CAP_FOWNER))
> +		return -EPERM;

Why? Firstly this does not handle user namespaces at all, secondly it
doesn't match the check done during ioctl, and thirdly vfs_fileattr_get()
should do all the needed checks?

> +
> +	dir = fdget(dfd);
> +	if (!fd_file(dir))
> +		return -EBADF;
> +
> +	if (!S_ISDIR(file_inode(fd_file(dir))->i_mode)) {
> +		error = -EBADF;
> +		goto out;
> +	}
> +
> +	error = user_path_at(dfd, filename, at_flags, &filepath);
> +	if (error)
> +		goto out;

I guess this is OK for now but allowing full flexibility of the "_at"
syscall (e.g. like setxattrat() does) would be preferred. Mostly so that
userspace programmer doesn't have to read manpage in detail and think
whether the particular combination of path arguments is supported by a
particular syscall. Admittedly VFS could make this a bit simpler. Currently
the boilerplate code that's needed in path_setxattrat() &
filename_setxattr() / file_setxattr() is offputting.

> +
> +	inode = filepath.dentry->d_inode;
> +	if (file_inode(fd_file(dir))->i_sb->s_magic != inode->i_sb->s_magic) {
> +		error = -EBADF;
> +		goto out_path;
> +	}

What's the motivation for this check?

> +
> +	error = vfs_fileattr_get(filepath.dentry, &fa);
> +	if (error)
> +		goto out_path;
> +
> +	if (copy_fsxattr_to_user(&fa, fsx))
> +		error = -EFAULT;
> +
> +out_path:
> +	path_put(&filepath);
> +out:
> +	fdput(dir);
> +	return error;
> +}
> +
> +SYSCALL_DEFINE4(setfsxattrat, int, dfd, const char __user *, filename,
> +		struct fsxattr *, fsx, int, at_flags)
> +{

Same comments as for getfsxattrat() apply here as well.

> -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;
>  
> @@ -574,6 +573,7 @@ static int copy_fsxattr_from_user(struct fileattr *fa,
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(copy_fsxattr_from_user);

I guess no need to export this function? The code you call it from cannot
be compiled as a module.

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR