linux-kernel - Re: [PATCH] fs/xattr: actually support O

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: <20250206-uhrwerk-faultiere-7a308565e2d3@brauner>
Date: Thu, 6 Feb 2025 10:31:54 +0100
From: Christian Brauner <brauner@...nel.org>
To: Mike Yuan <me@...dnzj.com>
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Al Viro <viro@...iv.linux.org.uk>, Christian Göttsche <cgzones@...glemail.com>, 
	stable@...r.kernel.org
Subject: Re: [PATCH] fs/xattr: actually support O_PATH fds in *xattrat()
 syscalls

On Wed, Feb 05, 2025 at 08:47:23PM +0000, Mike Yuan wrote:
> Cited from commit message of original patch [1]:
> 
> > One use case will be setfiles(8) setting SELinux file contexts
> > ("security.selinux") without race conditions and without a file
> > descriptor opened with read access requiring SELinux read permission.
> 
> Also, generally all *at() syscalls operate on O_PATH fds, unlike
> f*() ones. Yet the O_PATH fds are rejected by *xattrat() syscalls
> in the final version merged into tree. Instead, let's switch things
> to CLASS(fd_raw).
> 
> Note that there's one side effect: f*xattr() starts to work with
> O_PATH fds too. It's not clear to me whether this is desirable
> (e.g. fstat() accepts O_PATH fds as an outlier).
> 
> [1] https://lore.kernel.org/all/20240426162042.191916-1-cgoettsche@seltendoof.de/
> 
> Fixes: 6140be90ec70 ("fs/xattr: add *at family syscalls")
> Signed-off-by: Mike Yuan <me@...dnzj.com>
> Cc: Al Viro <viro@...iv.linux.org.uk>
> Cc: Christian Göttsche <cgzones@...glemail.com>
> Cc: Christian Brauner <brauner@...nel.org>
> Cc: <stable@...r.kernel.org>
> ---

I expanded on this before. O_PATH is intentionally limited in scope and
it should not allow to perform operations that are similar to a read or
write which getting and setting xattrs is.

Patches that further weaken or dilute the semantics of O_PATH are not
acceptable.

>  fs/xattr.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 02bee149ad96..15df71e56187 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -704,7 +704,7 @@ static int path_setxattrat(int dfd, const char __user *pathname,
>  
>  	filename = getname_maybe_null(pathname, at_flags);
>  	if (!filename) {
> -		CLASS(fd, f)(dfd);
> +		CLASS(fd_raw, f)(dfd);
>  		if (fd_empty(f))
>  			error = -EBADF;
>  		else
> @@ -848,7 +848,7 @@ static ssize_t path_getxattrat(int dfd, const char __user *pathname,
>  
>  	filename = getname_maybe_null(pathname, at_flags);
>  	if (!filename) {
> -		CLASS(fd, f)(dfd);
> +		CLASS(fd_raw, f)(dfd);
>  		if (fd_empty(f))
>  			return -EBADF;
>  		return file_getxattr(fd_file(f), &ctx);
> @@ -978,7 +978,7 @@ static ssize_t path_listxattrat(int dfd, const char __user *pathname,
>  
>  	filename = getname_maybe_null(pathname, at_flags);
>  	if (!filename) {
> -		CLASS(fd, f)(dfd);
> +		CLASS(fd_raw, f)(dfd);
>  		if (fd_empty(f))
>  			return -EBADF;
>  		return file_listxattr(fd_file(f), list, size);
> @@ -1079,7 +1079,7 @@ static int path_removexattrat(int dfd, const char __user *pathname,
>  
>  	filename = getname_maybe_null(pathname, at_flags);
>  	if (!filename) {
> -		CLASS(fd, f)(dfd);
> +		CLASS(fd_raw, f)(dfd);
>  		if (fd_empty(f))
>  			return -EBADF;
>  		return file_removexattr(fd_file(f), &kname);
> 
> base-commit: a86bf2283d2c9769205407e2b54777c03d012939
> -- 
> 2.48.1
> 
>