linux-kernel - Re: [PATCH] usb: gadget: udc-xilinx: validate ep number before indexing rambase[]

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: <2025070120-outhouse-lubricant-ff47@gregkh>
Date: Tue, 1 Jul 2025 10:43:50 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Sergio Pérez <sperezglz@...il.com>
Cc: michal.simek@....com, linux-usb@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	shuah@...nel.org
Subject: Re: [PATCH] usb: gadget: udc-xilinx: validate ep number before
 indexing rambase[]

On Mon, Jun 30, 2025 at 02:20:35PM -0600, Sergio Pérez wrote:
> > On Fri, Jun 27, 2025 at 12:01:22AM -0600, Sergio Perez Gonzalez wrote:
> > > Issue flagged by coverity. The size of the rambase array is 8,
> > > usb_enpoint_num() can return 0 to 15, prevent out of bounds reads.
> >
> > But how can that happen with this hardware?  As the array states, this
> > hardware only has that many endpoints availble to it, so how can it ever
> > be larger?
> >
> 
> Hardware will likely behave and not report more endpoints than it
> supports, but I thought that there is still a possibility that this
> can be exploited, taking into account this patch:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7f14c7227f342d9932f9b918893c8814f86d2a0d

That patch is probably not needed, for this same reason.

> and this CVE:
> https://www.cvedetails.com/cve/CVE-2022-27223/

Odds are we should reject that CVE, want me to go do that?

> However, looking more closely the above patch, the endpoint number is
> extracted from a struct different than the "usb_endpoint_descriptor":
> "epnum = udc->setup.wIndex & USB_ENDPOINT_NUMBER_MASK;"
> in contrast with the code that I'm touching. The CVE does not add more
> details to understand if the part of the code that I'm changing is not
> subject to the vulnerability.

Please dig deeper to determine this :)

> > > ---
> > >  drivers/usb/gadget/udc/udc-xilinx.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/usb/gadget/udc/udc-xilinx.c b/drivers/usb/gadget/udc/udc-xilinx.c
> > > index 8d803a612bb1..0c3714de2e3b 100644
> > > --- a/drivers/usb/gadget/udc/udc-xilinx.c
> > > +++ b/drivers/usb/gadget/udc/udc-xilinx.c
> > > @@ -814,6 +814,12 @@ static int __xudc_ep_enable(struct xusb_ep *ep,
> > >       ep->is_in = ((desc->bEndpointAddress & USB_DIR_IN) != 0);
> > >       /* Bit 3...0:endpoint number */
> > >       ep->epnumber = usb_endpoint_num(desc);
> > > +     if (ep->epnumber >= XUSB_MAX_ENDPOINTS) {
> > > +             dev_dbg(udc->dev, "bad endpoint index %d: only 0 to %d supported\n",
> > > +                             ep->epnumber, (XUSB_MAX_ENDPOINTS - 1));
> > > +             return -EINVAL;
> >
> > Any hints as to how this was tested?
> 
> I don't have access to such xilinx hardware, given that it was marked
> as a high severity defect in coverity and it is basically extending a
> validation that was already added in other parts of the code, I
> decided to propose the patch without runtime testing.

Never trust static analysis tools blindly.  The number of
false-positives stuff like coverity creates because it can not determine
where data actually comes from is way too high.

thanks,

greg k-h