Re: [PATCH v2 2/2] fuse: reject oversized payload_sz in fuse_uring_copy_from_ring()

[Joanne Koong]

On Wed, Jul 8, 2026 at 12:22 PM Bernd Schubert <bernd@xxxxxxxxxxx> wrote:
>
> Hi Xiang,
>
> On 7/7/26 20:44, Xiang Mei wrote:
> > fuse_uring_copy_from_ring() imports the payload buffer with length
> > ring->max_payload_sz but passes the server-controlled payload_sz to
> > fuse_copy_out_args() unchecked.  A larger payload_sz drains the iterator
> > to exhaustion and fuse_copy_fill() hits BUG_ON(!err), panicking the
> > kernel.  Reject replies whose payload_sz exceeds the imported buffer.
> >
> >   kernel BUG at fs/fuse/dev.c:1053!
> >   RIP: 0010:fuse_copy_fill+0x6c6/0x7e0
> >   Call Trace:
> >    fuse_copy_args
> >    fuse_uring_copy_from_ring     fs/fuse/dev_uring.c:686
> >    fuse_uring_cmd
> >    io_uring_cmd
> >    __io_issue_sqe
> >    io_submit_sqes
> >    __do_sys_io_uring_enter
> >    entry_SYSCALL_64_after_hwframe
> >
> > Fixes: c090c8abae4b ("fuse: Add io-uring sqe commit and fetch support")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Reported-by: Weiming Shi <bestswngs@xxxxxxxxx>
> > Assisted-by: Claude:claude-opus-4-8
> > Signed-off-by: Xiang Mei <xmei5@xxxxxxx>
> > Reviewed-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> > ---
> > v2: add: Cc stable and Reviewed-by tags
> >
> >  fs/fuse/dev_uring.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> > index 0814681eb04b..f6127c230dd9 100644
> > --- a/fs/fuse/dev_uring.c
> > +++ b/fs/fuse/dev_uring.c
> > @@ -679,6 +679,9 @@ static int fuse_uring_copy_from_ring(struct fuse_ring *ring,
> >       if (err)
> >               return err;
> >
> > +     if (ring_in_out.payload_sz > ring->max_payload_sz)
> > +             return -EINVAL;
> > +
> >       err = setup_fuse_copy_state(&cs, ring, req, ent, ITER_SOURCE, &iter);
> >       if (err)
> >               return err;
>
> Good catch and sorry for lare review! Hrmm, it just gives me a bit headache,
> because idea for max_payload_size in fuse_uring_create() that it prevents
> exactly that.

I don't think this is related to fuse_uring_create()'s
max_payload_size. The problem this fix addresses is that the
ring_ent_in_out.payload_sz header value returned by userspace can be
whatever arbitrary big value userspace wants to set.

Maybe I'm misunderstanding what you're saying, but i think this fix is
orthogonal to the setxattr issue you mention below? afaict, that one
has a different root cause (unbounded in_args copy coming from the
kernel side when sending requests vs. trying to copy in an unbounded
server-set payload size when handling a server's reply).

>
> After tracing through the code, I think we have two cases where max_payload calculation
> in fuse_uring_create() is not enough for xattr and ioctl
>
> For xattr we have an additional in addition to the patch above - it sends unchecked
> against max_pages and  fuse_dev_do_read() has an additional op code protection
> that I had missed

Nice find, I didn't realize setxattr had a special error value either

>
>        /* If request is too large, reply with an error and restart the read */
>         if (nbytes < reqsize) {
>                 req->out.h.error = -EIO;
>                 /* SETXATTR is special, since it may contain too large data */
>                 if (args->opcode == FUSE_SETXATTR)
>                         req->out.h.error = -E2BIG;
>                 fuse_request_end(req);
>                 goto restart;
>         }
>
>
>
> And I think with the current patch is incomplete and missing something like this
>
> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> index 77c8cec43d9c..449b84ac24e7 100644
> --- a/fs/fuse/dev_uring.c
> +++ b/fs/fuse/dev_uring.c
> @@ -725,6 +725,14 @@ static int fuse_uring_args_to_ring(struct fuse_ring *ring, struct fuse_req *req,
>                 num_args--;
>         }
>
> +       /*
> +        * A FUSE_SETXATTR value may exceed the ring buffer; match
> +        * fuse_dev_do_read() instead of overrunning the payload iterator.
> +        */
> +       if (fuse_len_args(num_args, (struct fuse_arg *)in_args) >
> +           ring->max_payload_sz)
> +               return args->opcode == FUSE_SETXATTR ? -E2BIG : -EIO;
> +
>         /* copy the payload */
>         err = fuse_copy_args(&cs, num_args, args->in_pages,
>                              (struct fuse_arg *)in_args, 0);
>
>
>
> And a generic patch, but that has the potential to break existing userspace is
>
> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> index 449b84ac24e7..d25d7922bbdd 100644
> --- a/fs/fuse/dev_uring.c
> +++ b/fs/fuse/dev_uring.c
> @@ -251,7 +251,14 @@ static struct fuse_ring *fuse_uring_create(struct fuse_chan *fch)
>                 goto out_err;
>
>         max_payload_size = max(FUSE_MIN_READ_BUFFER, fch->max_write);
> -       max_payload_size = max(max_payload_size, fch->max_pages * PAGE_SIZE);
> +       /*
> +        * A max_pages-sized paged reply may be preceded by a fixed op reply
> +        * header (e.g. FUSE_IOCTL); reserve a page of header room generically.
> +        */
> +       max_payload_size = max(max_payload_size,
> +                              fch->max_pages * PAGE_SIZE + PAGE_SIZE);

I hope we don't have to do this because imo even with a feature flag,
it gets confusing/cluttered. What about just having it be the
responsibility of userspace/libfuse to allocate a big enough buffer to
hold ioctl reply headers if they want to handle ioctls that have
max_payload_size amount of data? On the kernel side we'd just need to
store the buf_size value the user already passes in at registration
time and use that value for the import and payload-size bounds check
instead of max_payload_size which seems like a pretty minimal change.
If the kernel insists on the + PAGE_SIZE headroom, across lots of
queues that have lots of buffers each, I think that memory waste may
add up as well :(

I think with bufferpools being the primary interface going forward
(not sure if you agree with this, but with zero-copy and other
optimizations being gated on it, I view it as that in my mind), the
ioctl issue won't be a problem since the kernel can allocate more
memory than max_payload_size to that particular request from the pool.

Since they have alternative solutions (libfuse or buffer pools), maybe
we don't need to bake this in here?


> +       /* getxattr/listxattr values are bounded only by XATTR_SIZE_MAX */
> +       max_payload_size = max(max_payload_size, (size_t)XATTR_SIZE_MAX);

afaict XATTR_SIZE_MAX is 64k, so if a server deliberately sets max
pages to a very small value because they don't have enough memory, it
seems counter-intuitive to force all their buffers to be >= 64k. As I
understand it, the vast majority of xattrs are small (tens of bytes),
so maybe better to just let them proceed and return an error on an
oversized xattr than pay 64k/entry in every queue? If they really want
to support 64k xattrs, then imo it should be their responsibility to
allocate a big enough buffer size for that.

Thanks,
Joanne

>
>         spin_lock(&fch->lock);
>         if (!fch->connected) {
>
>
> Question is how we could add it in, maybe with a feature flag?
>
>
> Thanks,
> Bernd