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

[asmadeus]

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@xxxxxxxxx> 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@xxxxxxxxx
)


Thanks,
-- 
Dominique Martinet | Asmadeus