On Fri 25-04-25 10:45:22, Christian Brauner wrote:
> On Thu, Apr 24, 2025 at 05:45:17PM +0200, Mateusz Guzik wrote:
> > On Thu, Apr 24, 2025 at 03:22:47PM +0200, Jan Kara wrote:
> > > Currently, setxattrat(2) and getxattrat(2) are wrongly handling the
> > > calls of the from setxattrat(AF_FDCWD, NULL, AT_EMPTY_PATH, ...) and
> > > fail with -EBADF error instead of operating on CWD. Fix it.
> > >
> > > Fixes: 6140be90ec70 ("fs/xattr: add *at family syscalls")
> > > Signed-off-by: Jan Kara <jack@xxxxxxx>
> > > ---
> > > fs/xattr.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/xattr.c b/fs/xattr.c
> > > index 02bee149ad96..fabb2a04501e 100644
> > > --- a/fs/xattr.c
> > > +++ b/fs/xattr.c
> > > @@ -703,7 +703,7 @@ static int path_setxattrat(int dfd, const char __user *pathname,
> > > return error;
> > >
> > > filename = getname_maybe_null(pathname, at_flags);
> > > - if (!filename) {
> > > + if (!filename && dfd >= 0) {
> > > CLASS(fd, f)(dfd);
> > > if (fd_empty(f))
> > > error = -EBADF;
> > > @@ -847,7 +847,7 @@ static ssize_t path_getxattrat(int dfd, const char __user *pathname,
> > > return error;
> > >
> > > filename = getname_maybe_null(pathname, at_flags);
> > > - if (!filename) {
> > > + if (!filename && dfd >= 0) {
> > > CLASS(fd, f)(dfd);
> > > if (fd_empty(f))
> > > return -EBADF;
> >
> > Is there any code which legitimately does not follow this pattern?
> >
> > With some refactoring getname_maybe_null() could handle the fd thing,
> > notably return the NULL pointer if the name is empty. This could bring
> > back the invariant that the path argument is not NULL.
> >
> > Something like this:
> > static inline struct filename *getname_maybe_null(int fd, const char __user *name, int flags)
> > {
> > if (!(flags & AT_EMPTY_PATH))
> > return getname(name);
> >
> > if (!name && fd >= 0)
> > return NULL;
> > return __getname_maybe_null(fd, name);
> > }
> >
> > struct filename *__getname_maybe_null(int fd, const char __user *pathname)
> > {
> > char c;
> >
> > if (fd >= 0) {
> > /* try to save on allocations; loss on um, though */
> > if (get_user(c, pathname))
> > return ERR_PTR(-EFAULT);
> > if (!c)
> > return NULL;
> > }
> >
> > /* we alloc suffer the allocation of the buffer. worst case, if
> > * the name turned empty in the meantime, we return it and
> > * handle it the old-fashioned way.
> > /
> > return getname_flags(pathname, LOOKUP_EMPTY);
> > }
> >
> > Then callers would look like this:
> > filename = getname_maybe_null(dfd, pathname, at_flags);
> > if (!filename) {
> > /* fd handling goes here */
> > CLASS(fd, f)(dfd);
> > ....
> >
> > } else {
> > /* regular path handling goes here */
> > }
> >
> >
> > set_nameidata() would lose this branch:
> > p->pathname = likely(name) ? name->name : "";
> >
> > and putname would convert IS_ERR_OR_NULL (which is 2 branches) into one,
> > maybe like so:
> > - if (IS_ERR_OR_NULL(name))
> > + VFS_BUG_ON(!name);
> > +
> > + if (IS_ERR(name))
> > return;
> >
> > i think this would be an ok cleanup
>
> Not opposed, but please for -next and Jan's thing as a backportable fix,
> please. Thanks!
Exactly, I agree the code is pretty subtle and ugly. It shouldn't take
several engineers to properly call a function to lookup a file :) So
some cleanup and refactoring is definitely long overdue but for now I
wanted some minimal fix which is easy to backport to stable.
When we speak about refactoring: Is there a reason why user_path_at()
actually doesn't handle NULL 'name' as empty like we do it in *xattrat()
syscalls? I understand this will make all _at() syscalls accept NULL name
with AT_EMPTY_PATH but is that a problem?
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
