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: <aFCh-JXnifNXTgSt@codewreck.org>
Date: Tue, 17 Jun 2025 08:00:08 +0900
From: asmadeus@...ewreck.org
To: Yuhao Jiang <danisjiang@...il.com>
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

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)

> 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