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