[PATCH v3] usb: cdns3: validate endpoint index from setup packet

[PATCH v3] usb: cdns3: validate endpoint index from setup packet

[liuc63]

From: Liu Chao <liuc63@xiaopeng.com>

cdns3_ep_addr_to_index() masks the endpoint address with 0x7F instead of
USB_ENDPOINT_NUMBER_MASK (0x0F).  wIndex is supplied by the USB host, so
bits 4-6 of the endpoint number are attacker-controlled and the returned
index can reach 143, while priv_dev->eps[] holds only
CDNS3_ENDPOINTS_MAX_COUNT (32) entries.  Both cdns3_req_ep0_get_status()
(GET_STATUS) and cdns3_ep0_feature_handle_endpoint() (SET/CLEAR_FEATURE)
feed that index straight into priv_dev->eps[index] and dereference the
result, so a crafted control request causes an out-of-bounds access.
Even when the index stays within 0-31, the stray bits alias the request
onto a different endpoint slot.

Mask the endpoint number with USB_ENDPOINT_NUMBER_MASK so the computed
index is always within [0, 31].  This mirrors the cdnsp counterpart
cdnsp_w_index_to_ep_index(), which already extracts the endpoint number
with the same mask.

In addition, eps[index] may be NULL for endpoints that are not enabled,
as cdns3_init_eps() only populates entries for endpoints enabled in
USB_CAP3.  Check priv_dev->eps[index] for NULL before dereferencing it
in both functions, matching the fix already applied to aspeed_udc in
commit ee0d382feb44 ("usb: gadget: aspeed_udc: validate endpoint index
for ast udc").

Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
Cc: <stable@vger.kernel.org>
Signed-off-by: Liu Chao <liuc63@xiaopeng.com>
---
Changes in v3:
 - Mask the endpoint number with USB_ENDPOINT_NUMBER_MASK (0x0F) in
   cdns3_ep_addr_to_index() instead of bounding the 0x7F-derived index
   at the call sites; the index is now always within eps[], which also
   resolves the endpoint-aliasing concern raised by the Sashiko review
   (Peter Chen)
 - Drop the now-redundant index >= CDNS3_ENDPOINTS_MAX_COUNT check,
   keeping only the NULL check on priv_dev->eps[index] (Peter Chen)
 - The Sashiko review also flagged that the raw wIndex is passed
   unmasked to cdns3_select_ep() and so reaches the ep_sel register;
   that is pre-existing behaviour (commit 7733f6c32e36), orthogonal
   to this fix, and is deferred to a separate follow-up patch

Changes in v2:
 - Also check priv_dev->eps[index] for NULL, as endpoints not enabled
   in USB_CAP3 are left NULL by cdns3_init_eps() (Peter Chen)
 - Correct the reference: the same-class fix ee0d382feb44 is for
   aspeed_udc, not renesas_usb3; cite it with the commit title and
   drop the CVE mention (Peter Chen)
 - Fix the function name cdns3_ep0_handle_status() ->
   cdns3_req_ep0_get_status() in the commit message

v2: https://lore.kernel.org/linux-usb/20261012110107.2458022-1-liuc63@xiaopeng.com/
v1: https://lore.kernel.org/linux-usb/20260917103145.3047850-1-liuc63@xiaopeng.com/
---
 drivers/usb/cdns3/cdns3-ep0.c    | 4 ++++
 drivers/usb/cdns3/cdns3-gadget.c | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/cdns3/cdns3-ep0.c b/drivers/usb/cdns3/cdns3-ep0.c
index e29989d57bef..ab18adec831e 100644
--- a/drivers/usb/cdns3/cdns3-ep0.c
+++ b/drivers/usb/cdns3/cdns3-ep0.c
@@ -251,6 +251,8 @@ static int cdns3_req_ep0_get_status(struct cdns3_device *priv_dev,
 		return cdns3_ep0_delegate_req(priv_dev, ctrl);
 	case USB_RECIP_ENDPOINT:
 		index = cdns3_ep_addr_to_index(le16_to_cpu(ctrl->wIndex));
+		if (!priv_dev->eps[index])
+			return -EINVAL;
 		priv_ep = priv_dev->eps[index];
 
 		/* check if endpoint is stalled or stall is pending */
@@ -368,6 +370,8 @@ static int cdns3_ep0_feature_handle_endpoint(struct cdns3_device *priv_dev,
 		return 0;
 
 	index = cdns3_ep_addr_to_index(le16_to_cpu(ctrl->wIndex));
+	if (!priv_dev->eps[index])
+		return -EINVAL;
 	priv_ep = priv_dev->eps[index];
 
 	cdns3_select_ep(priv_dev, le16_to_cpu(ctrl->wIndex));
diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
index 0d272e9c556c..f65a87f11e26 100644
--- a/drivers/usb/cdns3/cdns3-gadget.c
+++ b/drivers/usb/cdns3/cdns3-gadget.c
@@ -109,7 +109,8 @@ void cdns3_set_register_bit(void __iomem *ptr, u32 mask)
  */
 u8 cdns3_ep_addr_to_index(u8 ep_addr)
 {
-	return (((ep_addr & 0x7F)) + ((ep_addr & USB_DIR_IN) ? 16 : 0));
+	return ((ep_addr & USB_ENDPOINT_NUMBER_MASK) +
+		((ep_addr & USB_DIR_IN) ? 16 : 0));
 }
 
 static int cdns3_get_dma_pos(struct cdns3_device *priv_dev,
-- 
2.50.1

Re: [PATCH v3] usb: cdns3: validate endpoint index from setup packet

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: cdns3: ep0 guard bypass in cdns3_ep0_feature_handle_endpoint()
--

commit 315547ec5e1cd4b938b672e53824e1e53cb91496
Author: Liu Chao <liuc63@xiaopeng.com>

usb: cdns3: validate endpoint index from setup packet

This commit masks the endpoint number in cdns3_ep_addr_to_index() with
USB_ENDPOINT_NUMBER_MASK (0x0F) instead of 0x7F and adds NULL checks on
priv_dev->eps[index] in cdns3_req_ep0_get_status() and
cdns3_ep0_feature_handle_endpoint().

> diff --git a/drivers/usb/cdns3/cdns3-ep0.c b/drivers/usb/cdns3/cdns3-ep0.c
> index e29989d57bef9..ab18adec831ea 100644
> --- a/drivers/usb/cdns3/cdns3-ep0.c
> +++ b/drivers/usb/cdns3/cdns3-ep0.c

[ ... ]

> @@ -368,6 +370,8 @@ static int cdns3_ep0_feature_handle_endpoint(struct cdns3_device *priv_dev,
>  		return 0;
>  
>  	index = cdns3_ep_addr_to_index(le16_to_cpu(ctrl->wIndex));
> +	if (!priv_dev->eps[index])
> +		return -EINVAL;
>  	priv_ep = priv_dev->eps[index];
>  
>  	cdns3_select_ep(priv_dev, le16_to_cpu(ctrl->wIndex));
> diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
> index f4dfd01317083..be6d607f55c92 100644
> --- a/drivers/usb/cdns3/cdns3-gadget.c
> +++ b/drivers/usb/cdns3/cdns3-gadget.c
> @@ -109,7 +109,8 @@ void cdns3_set_register_bit(void __iomem *ptr, u32 mask)
>   */
>  u8 cdns3_ep_addr_to_index(u8 ep_addr)
>  {
> -	return (((ep_addr & 0x7F)) + ((ep_addr & USB_DIR_IN) ? 16 : 0));
> +	return ((ep_addr & USB_ENDPOINT_NUMBER_MASK) +
> +		((ep_addr & USB_DIR_IN) ? 16 : 0));
>  }
>  
>  static int cdns3_get_dma_pos(struct cdns3_device *priv_dev,

[Severity: Medium]
Can a crafted SET_FEATURE or CLEAR_FEATURE (USB_ENDPOINT_HALT) request with
(wIndex & USB_ENDPOINT_NUMBER_MASK) == 0 and stray bits set in 0xff70 (for
example, wIndex = 0x0010 or 0x0100) now bypass the endpoint 0 check in
cdns3_ep0_feature_handle_endpoint() and operate on priv_dev->eps[0]?

In drivers/usb/cdns3/cdns3-ep0.c, cdns3_ep0_feature_handle_endpoint() guards
against endpoint 0 with:

	if (!(le16_to_cpu(ctrl->wIndex) & ~USB_DIR_IN))
		return 0;

	index = cdns3_ep_addr_to_index(le16_to_cpu(ctrl->wIndex));
	if (!priv_dev->eps[index])
		return -EINVAL;
	priv_ep = priv_dev->eps[index];

When wIndex has bits [3:0] equal to 0 and any bit in 0xff70 set,
(le16_to_cpu(ctrl->wIndex) & ~USB_DIR_IN) is non-zero, so the early return
is not taken. With cdns3_ep_addr_to_index() now masking with
USB_ENDPOINT_NUMBER_MASK (0x0f), index resolves to 0 or 16.

In drivers/usb/cdns3/cdns3-gadget.c:cdns3_init_eps(), both priv_dev->eps[0]
and priv_dev->eps[16] are populated with the EP0 endpoint pointer:

		if (ep_dir && !ep_number) {
			priv_dev->eps[i] = priv_dev->eps[0];
			continue;
		}

As a result, !priv_dev->eps[index] is false and priv_ep resolves to
priv_dev->eps[0]. Since ep_sel only decodes bits [3:0] (EP_SEL_EPNO_MASK)
and bit 7 (EP_SEL_DIR), cdns3_select_ep() selects EP0 in hardware and calls
__cdns3_gadget_ep_set_halt() or __cdns3_gadget_ep_clear_halt() on EP0. This
either stalls/flushes EP0 and sets sticky EP_STALLED / EP_STALL_PENDING
flags on priv_dev->eps[0]->flags (which cdns3-ep0.c never clears, causing
subsequent GET_STATUS requests on EP0 to report halted status) or issues
EP_CMD_CSTALL | EP_CMD_EPRST on EP0 during an active control transfer.

Should the endpoint 0 check in cdns3_ep0_feature_handle_endpoint() also mask
wIndex with USB_ENDPOINT_NUMBER_MASK (or check !(index & ~16)) so it stays
consistent with cdns3_ep_addr_to_index()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017024422.3400907-1-liuc63@xiaopeng.com?part=1