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: <Kn-2tlpxN8YNmh0j0udjQ8YIFNZMVVJYJh-LyBoYNBfpax28PNUkXuH6gnod7if2eX3NRVs3Ey8uCHRpwg_S5hPX_ADtrgMZbDTWFpHd_uU=@yhndnzj.com>
Date: Thu, 06 Feb 2025 09:51:33 +0000
From: Mike Yuan <me@...dnzj.com>
To: "brauner@...nel.org" <brauner@...nel.org>
Cc: "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>, "cgzones@...glemail.com" <cgzones@...glemail.com>, "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH] fs/xattr: actually support O_PATH fds in *xattrat() syscalls

On 2/6/25 10:31, Christian Brauner <brauner@...nel.org> wrote:

>  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.

But the *at() variants really should be able to work with O_PATH fds, otherwise they're basically useless? I guess I just need to keep f*() as-is?

>  >  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
>  >
>  >
>