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 for Android: free password hash cracker in your pocket

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

Message-ID: <20250206-erbauen-ornament-26f338d98f13@brauner>
Date: Thu, 6 Feb 2025 11:03:06 +0100
From: Christian Brauner <brauner@...nel.org>
To: Mike Yuan <me@...dnzj.com>
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 Thu, Feb 06, 2025 at 09:51:33AM +0000, Mike Yuan wrote:
> 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?

I'm confused. If you have:

        filename = getname_maybe_null(pathname, at_flags);
        if (!filename) {
                CLASS(fd, f)(dfd);
                if (fd_empty(f))
                        error = -EBADF;
                else
                        error = file_setxattr(fd_file(f), &ctx);

Then this branch ^^ cannot use fd_raw because you're allowing operations
directly on the O_PATH file descriptor.

Using the O_PATH file descriptor for lookup is obviously fine which is
why the other branch exists:

        } else {
                error = filename_setxattr(dfd, filename, lookup_flags, &ctx);
        }

IOW, your patch makes AT_EMPTY_PATH work with an O_PATH file descriptor
which isn't acceptable. However, it is already perfectly fine to use an
O_PATH file descriptor for lookup.

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