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@codewre...
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,
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 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...)
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
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,
linux-stable-mirror@lists.linaro.org
-
asmadeus＠codewreck.org -
Christian Schoenebeck -
Dominique Martinet via B4 Relay -
Kees Cook