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

Greg KH
2025-07-01T08:53:10Z

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