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

On Wed, Jul 8, 2026 at 4:08 PM Joanne Koong <[email protected]> wrote:
>
> On Wed, Jul 8, 2026 at 12:22 PM Bernd Schubert <[email protected]> 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: [email protected]
> > > Reported-by: Weiming Shi <[email protected]>
> > > Assisted-by: Claude:claude-opus-4-8
> > > Signed-off-by: Xiang Mei <[email protected]>
> > > Reviewed-by: Joanne Koong <[email protected]>
> > > ---
> > > 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
>
Good catch, that's a real issue, and we triggered that by modifying our PoC.
> >
> >        /* 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

This seems a good way to patch both paths. We have sent a v3 based on this.

v3-0001 keeps the same as v2.
v3-0002 patches both paths based on your proposal.

Please check, and we'd like to hear your feedback:
https://lore.kernel.org/fuse-devel/[email protected]/T/#u

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