On Mon, Jun 30, 2025 at 02:20:35PM -0600, Sergio Pérez wrote:

quoted

On Fri, Jun 27, 2025 at 12:01:22AM -0600, Sergio Perez Gonzalez wrote:

quoted

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 :)

quoted

quoted

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