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
