On Tue, Jun 17, 2025 at 12:12 PM asmadeus@codewreck.org wrote:
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@gmail.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
Sure, you can do it and add me as Reported-by, thanks!