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] [day] [month] [year] [list]

Message-ID: <CAHYQsXRhFMEhgPvY_6PVjD=oiWf3DWgLCr78xy-fWvxZwAMT3w@mail.gmail.com>
Date: Tue, 17 Jun 2025 12:29:58 +0800
From: Danis Jiang <danisjiang@...il.com>
To: asmadeus@...ewreck.org
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

On Tue, Jun 17, 2025 at 12:12 PM <asmadeus@...ewreck.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@...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

Sure, you can do it and add me as Reported-by, thanks!