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

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

Posted by Sergio Perez Gonzalez 3 months, 1 week ago

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.

Link: https://scan7.scan.coverity.com/#/project-view/53936/11354?selectedIssue=1644635
Signed-off-by: Sergio Perez Gonzalez <sperezglz@gmail.com>
---
 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;
+	}
+
 	ep->desc = desc;
 	ep->ep_usb.desc = desc;
 	tmp = usb_endpoint_type(desc);
-- 
2.43.0

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

Posted by Greg KH 3 months, 1 week ago

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?

> Link: https://scan7.scan.coverity.com/#/project-view/53936/11354?selectedIssue=1644635
> Signed-off-by: Sergio Perez Gonzalez <sperezglz@gmail.com>

What commit id does this fix?


> ---
>  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?

thanks,

greg k-h

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

Posted by Sergio Pérez 3 months, 1 week ago

> 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

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

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.

> > Link: https://scan7.scan.coverity.com/#/project-view/53936/11354?selectedIssue=1644635
> > Signed-off-by: Sergio Perez Gonzalez <sperezglz@gmail.com>
>
> What commit id does this fix?

The last commit that touches this code is : fd2f928a5f7bc2f9588 ("usb:
gadget: udc-xilinx: Use USB API functions rather than constants") ,
although, I think the previous code gives functionally the same
behavior.

>
>
> > ---
> >  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.

Thanks,
Sergio
>
> thanks,
>
> greg k-h

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

Posted by Greg KH 3 months, 1 week ago

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