linux-kernel - Re: [PATCH] net/9p: Fix buffer overflow in USB transport layer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHYQsXR43MGM826eHtEkmH4X2bM-amM29A38XUj+hMbNF2vDJQ@mail.gmail.com>
Date: Tue, 17 Jun 2025 11:01:40 +0800
From: Danis Jiang <danisjiang@...il.com>
To: asmadeus@...ewreck.org
Cc: ericvh@...nel.org, lucho@...kov.net, linux_oss@...debyte.com, 
	v9fs@...ts.linux.dev, linux-kernel@...r.kernel.org, security@...nel.org, 
	stable@...r.kernel.org
Subject: Re: [PATCH] net/9p: Fix buffer overflow in USB transport layer

On Tue, Jun 17, 2025 at 7:00 AM <asmadeus@...ewreck.org> wrote:
>
> Yuhao Jiang wrote on Mon, Jun 16, 2025 at 09:25:39PM +0800:
> > A buffer overflow vulnerability exists in the USB 9pfs transport layer
> > where inconsistent size validation between packet header parsing and
> > actual data copying allows a malicious USB host to overflow heap buffers.
> >
> > The issue occurs because:
> > - usb9pfs_rx_header() validates only the declared size in packet header
> > - usb9pfs_rx_complete() uses req->actual (actual received bytes) for memcpy
> >
> > This allows an attacker to craft packets with small declared size (bypassing
> > validation) but large actual payload (triggering overflow in memcpy).
> >
> > Add validation in usb9pfs_rx_complete() to ensure req->actual does not
> > exceed the buffer capacity before copying data.
>
> Thanks for this check!
>
> Did you reproduce this or was this static analysis found?
> (to knowi if you tested wrt question below)

I found this by static analysis.

>
> > Reported-by: Yuhao Jiang <danisjiang@...il.com>
> > Fixes: a3be076dc174 ("net/9p/usbg: Add new usb gadget function transport")
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Yuhao Jiang <danisjiang@...il.com>
> > ---
> >  net/9p/trans_usbg.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/net/9p/trans_usbg.c b/net/9p/trans_usbg.c
> > index 6b694f117aef..047a2862fc84 100644
> > --- a/net/9p/trans_usbg.c
> > +++ b/net/9p/trans_usbg.c
> > @@ -242,6 +242,15 @@ static void usb9pfs_rx_complete(struct usb_ep *ep, struct usb_request *req)
> >       if (!p9_rx_req)
> >               return;
> >
> > +     /* Validate actual received size against buffer capacity */
> > +     if (req->actual > p9_rx_req->rc.capacity) {
> > +             dev_err(&cdev->gadget->dev,
> > +                     "received data size %u exceeds buffer capacity %zu\n",
> > +                     req->actual, p9_rx_req->rc.capacity);
> > +             p9_req_put(usb9pfs->client, p9_rx_req);
>
> I still haven't gotten around to setting up something to test this, and
> even less the error case, but I'm not sure a single put is enough --
> p9_client_cb does another put.
> Conceptually I think it's better to mark the error and move on
> e.g. (not even compile tested)
> ```
>         int status = REQ_STATUS_RCVD;
>
>         [...]
>
>         if (req->actual > p9_rx_req->rc.capacity) {
>                 dev_err(...)
>                 req->actual = 0;
>                 status = REQ_STATUS_ERROR;
>         }
>
>         memcpy(..)
>
>         p9_rx_req->rc.size = req->actual;
>
>         p9_client_cb(usb9pfs->client, p9_rx_req, status);
>         p9_req_put(usb9pfs->client, p9_rx_req);
>
>         complete(&usb9pfs->received);
> ```
> (I'm not sure overriding req->actual is allowed, might be safer to use
> an intermediate variable like status instead)
>
> What do you think?
>
> Thanks,
> --
> Dominique Martinet | Asmadeus

Yes, I think your patch is better, my initial patch forgot p9_client_cb.

Thanks,
Yuhao Jiang