[PATCH v3] net/9p: Fix buffer overflow in USB transport layer

[PATCH v3] net/9p: Fix buffer overflow in USB transport layer

[Dominique Martinet via B4 Relay]

From: Dominique Martinet <asmadeus@codewreck.org>

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.

Reported-by: Yuhao Jiang <danisjiang@gmail.com>
Closes: https://lkml.kernel.org/r/20250616132539.63434-1-danisjiang@gmail.com
Fixes: a3be076dc174 ("net/9p/usbg: Add new usb gadget function transport")
Cc: stable@vger.kernel.org
Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---
(still not actually tested, can't get dummy_hcd/gt to create a device
listed by p9_fwd.py/useable in qemu, I give up..)

Changes in v3:
- fix typo s/req_sizel/req_size/ -- sorry for that, module wasn't
  built...
- Link to v2: https://lore.kernel.org/r/20250620-9p-usb_overflow-v2-1-026c6109c7a1@codewreck.org

Changes in v2:
- run through p9_client_cb() on error
- Link to v1: https://lore.kernel.org/r/20250616132539.63434-1-danisjiang@gmail.com
---
 net/9p/trans_usbg.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/net/9p/trans_usbg.c b/net/9p/trans_usbg.c
index 6b694f117aef296a66419fed5252305e7a1d0936..468f7e8f0277b9ae5f1bb3c94c649fca97d28857 100644
--- a/net/9p/trans_usbg.c
+++ b/net/9p/trans_usbg.c
@@ -231,6 +231,8 @@ static void usb9pfs_rx_complete(struct usb_ep *ep, struct usb_request *req)
 	struct f_usb9pfs *usb9pfs = ep->driver_data;
 	struct usb_composite_dev *cdev = usb9pfs->function.config->cdev;
 	struct p9_req_t *p9_rx_req;
+	unsigned int req_size = req->actual;
+	int status = REQ_STATUS_RCVD;
 
 	if (req->status) {
 		dev_err(&cdev->gadget->dev, "%s usb9pfs complete --> %d, %d/%d\n",
@@ -242,11 +244,19 @@ static void usb9pfs_rx_complete(struct usb_ep *ep, struct usb_request *req)
 	if (!p9_rx_req)
 		return;
 
-	memcpy(p9_rx_req->rc.sdata, req->buf, req->actual);
+	if (req_size > p9_rx_req->rc.capacity) {
+		dev_err(&cdev->gadget->dev,
+			"%s received data size %u exceeds buffer capacity %zu\n",
+			ep->name, req_size, p9_rx_req->rc.capacity);
+		req_size = 0;
+		status = REQ_STATUS_ERROR;
+	}
 
-	p9_rx_req->rc.size = req->actual;
+	memcpy(p9_rx_req->rc.sdata, req->buf, req_size);
 
-	p9_client_cb(usb9pfs->client, p9_rx_req, REQ_STATUS_RCVD);
+	p9_rx_req->rc.size = req_size;
+
+	p9_client_cb(usb9pfs->client, p9_rx_req, status);
 	p9_req_put(usb9pfs->client, p9_rx_req);
 
 	complete(&usb9pfs->received);

---
base-commit: 74b4cc9b8780bfe8a3992c9ac0033bf22ac01f19
change-id: 20250620-9p-usb_overflow-25bfc5e9bef3

Best regards,
-- 
Dominique Martinet <asmadeus@codewreck.org>

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

[Kees Cook]

On June 22, 2025 6:39:56 AM PDT, Dominique Martinet via B4 Relay <devnull+asmadeus.codewreck.org@kernel.org> wrote:
> [...]
>Add validation in usb9pfs_rx_complete() to ensure req->actual does not
>exceed the buffer capacity before copying data.
> [...]
>+	if (req_size > p9_rx_req->rc.capacity) {
>+		dev_err(&cdev->gadget->dev,
>+			"%s received data size %u exceeds buffer capacity %zu\n",
>+			ep->name, req_size, p9_rx_req->rc.capacity);
>+		req_size = 0;
>+		status = REQ_STATUS_ERROR;
>+	}
> 
>-	p9_rx_req->rc.size = req->actual;
>+	memcpy(p9_rx_req->rc.sdata, req->buf, req_size);

Is rc.sdata always rc.capacity sized? If so, this world be a good first adopter of the __counted_by annotation for pointer struct members, available in Clang trunk and soon in GCC:
https://gcc.gnu.org/pipermail/gcc-patches/2025-May/683696.html

-Kees

 

-- 
Kees Cook

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

[asmadeus]

Kees Cook wrote on Sun, Jun 22, 2025 at 01:02:20PM -0700:
> >-	p9_rx_req->rc.size = req->actual;
> >+	memcpy(p9_rx_req->rc.sdata, req->buf, req_size);
> 
> Is rc.sdata always rc.capacity sized? If so, this world be a good first adopter of the __counted_by annotation for pointer struct members, available in Clang trunk and soon in GCC:
> https://gcc.gnu.org/pipermail/gcc-patches/2025-May/683696.html

I think so, I'll add the annotation in another patch when time allows
(and try to revert this commit to check it works, even if I have no
reason to believe it wouldn't catch this)

(... And this made me realize commit 60ece0833b6c ("net/9p: allocate
appropriate reduced message buffers") likely broke everything for
9p/rdma 3 years ago, as rdma is swapping buffers around...
I guess it doesn't have (m)any users...)

-- 
Dominique Martinet | Asmadeus

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

[Christian Schoenebeck]

On Sunday, June 22, 2025 10:39:29 PM CEST asmadeus@codewreck.org wrote:
[...]
> (... And this made me realize commit 60ece0833b6c ("net/9p: allocate
> appropriate reduced message buffers") likely broke everything for
> 9p/rdma 3 years ago, as rdma is swapping buffers around...
> I guess it doesn't have (m)any users...)

That patch contains an RDMA exception:

@@ -645,9 +664,18 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
        int sigpending, err;
        unsigned long flags;
        struct p9_req_t *req;
+       /* Passing zero for tsize/rsize to p9_client_prepare_req() tells it to
+        * auto determine an appropriate (small) request/response size
+        * according to actual message data being sent. Currently RDMA
+        * transport is excluded from this response message size optimization,
+        * as it would not cope with it, due to its pooled response buffers
+        * (using an optimized request size for RDMA as well though).
+        */
+       const uint tsize = 0;
+       const uint rsize = c->trans_mod->pooled_rbuffers ? c->msize : 0;
 
        va_start(ap, fmt);
-       req = p9_client_prepare_req(c, type, c->msize, c->msize, fmt, ap);
+       req = p9_client_prepare_req(c, type, tsize, rsize, fmt, ap);
        va_end(ap);
        if (IS_ERR(req))
                return req;

/Christian

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

[asmadeus]

Christian Schoenebeck wrote on Sun, Jun 22, 2025 at 11:20:21PM +0200:
> On Sunday, June 22, 2025 10:39:29 PM CEST asmadeus@codewreck.org wrote:
> [...]
> > (... And this made me realize commit 60ece0833b6c ("net/9p: allocate
> > appropriate reduced message buffers") likely broke everything for
> > 9p/rdma 3 years ago, as rdma is swapping buffers around...
> > I guess it doesn't have (m)any users...)
> 
> That patch contains an RDMA exception:

Oh, thanks for pointing that out!


BTW I just tried __counted_by and it's not obvious because it's not
allocated with the fcall (fcall structs themselves are allocated in the
req, and each fcall gets a data buffer)

For everything other than RDMA it shouldn't be too difficult to bubble
the allocation up (fcall+data as a flexible array as a pointer in req),
but then with large "round" msizes we'd get into the next power of two
buckets so I think it's probably better to keep as is.
(.. that and I wouldn't look forward to rework the buffer swapping logic
with RDMA, even if it should be straightforward enough with a couple of
container_of()s...)

Perhaps when/if counted_by learns to apply to pointers:
---
.../include/net/9p/9p.h:558:13: error: ‘counted_by’ attribute is not allowed for a non-array field
  558 |         u8 *sdata __counted_by(capacity);
      |             ^~~~~
make[3]: *** [.../scripts/Makefile.build:287: trans_xen.o] Error 1
In file included from client.c:22:
---

Thanks,
-- 
Dominique Martinet | Asmadeus