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

[Jens Axboe <axboe@kernel.dk>]

On 10/5/24 11:28 PM, Al Viro wrote:
> 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.

Sounds like a plan.

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

I just checked, and while I think this is fine to do for the 'fd' taking
{s,g}etxattr, I don't think the path taking ones should allow
IOSQE_FIXED_FILE being set. It's nonsensical, as they don't take a file
descriptor. So I'd prefer if we kept it to just the f* variants. I can
just make this tweak in my io_uring 6.12 branch and get it upstream this
week, that'll take it out of your hands.

What do you think?

-- 
Jens Axboe