On Wed, Oct 02, 2024 at 04:55:22PM -0600, Jens Axboe wrote:
> The reason I liked the putname() is that it's unconditional - the caller
> can rely on it being put, regardless of the return value. So I'd say the
> same should be true for ctx.kvalue, and if not, the caller should still
> free it. That's the path of least surprise - no leak for the least
> tested error path, and no UAF in the success case.
The problem with ctx.kvalue is that on the syscall side there's a case when
we do not call either file_setxattr() or filename_setxattr() - -EBADF.
And it's a lot more convenient to do setxattr_copy() first, so we end
up with a lovely landmine:
filename = getname_xattr(pathname, at_flags);
if (!filename) {
CLASS(fd, f)(dfd);
if (fd_empty(f)) {
kfree(ctx.kvalue); // lest we leak
return -EBADF;
}
return file_setxattr(fd_file(f), &ctx);
}
return filename_setxattr(dfd, filename, lookup_flags, &ctx);
That's asking for trouble, obviously. So I think we ought to consume
filename (in filename_...()) case, leave struct file reference alone
(we have to - it might have been borrowed rather than cloned) and leave
->kvalue unchanged. Yes, it ends up being more clumsy, but at least
it's consistent between the cases...
As for consuming filename... On the syscall side it allows things like
SYSCALL_DEFINE3(mkdirat, int, dfd, const char __user *, pathname, umode_t, mode)
{
return do_mkdirat(dfd, getname(pathname), mode);
}
which is better than the alternatives - I mean, that's
SYSCALL_DEFINE3(mkdirat, int, dfd, const char __user *, pathname, umode_t, mode)
{
struct filename *filename = getname(pathname);
int res = do_mkdirat(dfd, filename, mode);
putname(filename);
return ret;
}
or
SYSCALL_DEFINE3(mkdirat, int, dfd, const char __user *, pathname, umode_t, mode)
{
struct filename *filename __free(putname) = getname(pathname);
return do_mkdirat(dfd, filename, mode);
}
and both stink, if for different reasons ;-/ Having those things consume
(unconditionally) is better, IMO.
Hell knows; let's go with what I described above for now and see where it leads
when more such helpers are regularized.
> That's a bit different than your putname() case, but I think as long as
> it's consistent regardless of return value, then either approach is
> fine. Maybe just add a comment about that? At least for the consistent
> case, if it blows up, it'll blow up instantly rather than be a surprise
> down the line for "case x,y,z doesn't put it" or "case x,y,z always puts
> in, normal one does not".
Obviously.
> > Questions on the io_uring side:
> > * you usually reject REQ_F_FIXED_FILE for ...at() at ->prep() time.
> > Fine, but... what's the point of doing that in IORING_OP_FGETXATTR case?
> > Or IORING_OP_GETXATTR, for that matter, since you pass AT_FDCWD anyway...
> > Am I missing something subtle here?
>
> Right, it could be allowed for fgetxattr on the io_uring side. Anything
> that passes in a struct file would be fair game to enable it on.
> Anything that passes in a path (eg a non-fd value), it obviously
> wouldn't make sense anyway.
OK, done and force-pushed into #work.xattr.
