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
