Re: [PATCH 5/9] replace do_setxattr() with saner helpers.

[Al Viro <viro@zeniv.linux.org.uk>]

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.