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: <aFDrBwql20jYrvp1@codewreck.org>
Date: Tue, 17 Jun 2025 13:11:51 +0900
From: asmadeus@...ewreck.org
To: Danis 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,
	Mirsad Todorovac <mtodorovac69@...il.com>
Subject: Re: [PATCH] net/9p: Fix buffer overflow in USB transport layer

Danis Jiang wrote on Tue, Jun 17, 2025 at 11:01:40AM +0800:
>>> 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.

Ok.

>> 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?
> 
> Yes, I think your patch is better, my initial patch forgot p9_client_cb.

Ok, let's go with that then.

Would you like to resend "my" version, or should I do it (and
refer to your patch as Reported-by)?

Also if you resend let's add Mirsad Todorovac <mtodorovac69@...il.com> too Ccs,
I've added him now.
(Mirsad, please check lore for full context if quote wasn't enough:
 https://lkml.kernel.org/r/20250616132539.63434-1-danisjiang@gmail.com
)


Thanks,
-- 
Dominique Martinet | Asmadeus