Re: [PATCH] fs/xattr: actually support O_PATH fds in *xattrat() syscalls

[Mike Yuan]

On 2/6/25 11:03, Christian Brauner <brauner@xxxxxxxxxx> wrote:

>  On Thu, Feb 06, 2025 at 09:51:33AM +0000, Mike Yuan wrote:
>  > On 2/6/25 10:31, Christian Brauner <brauner@xxxxxxxxxx> 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@xxxxxxxxxxxxx/
>  > >  >
>  > >  > Fixes: 6140be90ec70 ("fs/xattr: add *at family syscalls")
>  > >  > Signed-off-by: Mike Yuan <me@xxxxxxxxxxx>
>  > >  > Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
>  > >  > Cc: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
>  > >  > Cc: Christian Brauner <brauner@xxxxxxxxxx>
>  > >  > Cc: <stable@xxxxxxxxxxxxxxx>
>  > >  > ---
>  > >
>  > >  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.

Well, again, [1] clearly stated the use case:

> Those can be used to operate on extended attributes,
especially security related ones, either relative to a pinned directory
or [on a file descriptor without read access, avoiding a
/proc/<pid>/fd/<fd> detour, requiring a mounted procfs].

And this surfaced in my PR to systemd:

https://github.com/systemd/systemd/pull/36228/commits/34fe16fb177d2f917570c5f71dfa8f5b9746b9a7

How are *xattrat() syscalls different from e.g. fchmodat2(AT_EMPTY_PATH) in that regard? I can agree that the semantics of f*xattr() ought to be left untouched, yet I fail to grok the case for _at variants.

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