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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: cdns3: endpoint aliasing allows OUT requests to corrupt IN endpoint state
- [Medium] usb: cdns3: unmasked wIndex writes to ep_sel hardware register
--

commit 7991bca72d7e15fa3f3ecc88593a0461276be999
Author: Liu Chao <liuc63@xiaopeng.com>

usb: cdns3: validate endpoint index from setup packet

The commit adds array bounds and NULL checks for the endpoint index derived
from wIndex to prevent out-of-bounds array access and NULL pointer
dereferences when handling setup packets.

> diff --git a/drivers/usb/cdns3/cdns3-ep0.c b/drivers/usb/cdns3/cdns3-ep0.c
> index e29989d57bef9..d26fa89b6fc65 100644
> --- a/drivers/usb/cdns3/cdns3-ep0.c
> +++ b/drivers/usb/cdns3/cdns3-ep0.c
> @@ -251,6 +251,9 @@ 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 (index >= CDNS3_ENDPOINTS_MAX_COUNT ||
> +		    !priv_dev->eps[index])
> +			return -EINVAL;

[Severity: High]
Does this bounds check fully prevent endpoint aliasing?

Indices 16-31 in the eps array are reserved for IN endpoints. The newly
added check allows index values up to 31 to pass validation.

If a malicious USB host sends a standard control request with a malformed
wIndex where the endpoint direction is OUT (bit 7 is 0) but bits 4-6 are
set such that wIndex & 0x7F is between 16 and 31 (for example,
wIndex = 0x11), cdns3_ep_addr_to_index() evaluates to an index in the
16-31 range.

This bypasses the validation and maps an OUT request to the software
structure (priv_ep) of a valid IN endpoint, allowing an attacker to query
the software status of IN endpoints using malformed OUT requests.

>  		priv_ep = priv_dev->eps[index];
>  
>  		/* check if endpoint is stalled or stall is pending */

[ ... ]

> @@ -368,6 +371,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 (index >= CDNS3_ENDPOINTS_MAX_COUNT || !priv_dev->eps[index])
> +		return -EINVAL;

[Severity: High]
Similarly to the get_status function above, could the endpoint aliasing
allow a malformed OUT request to pass validation and erroneously modify the
software halt state of an IN endpoint?

If a malformed wIndex bypasses this check, the code will later call
__cdns3_gadget_ep_set_halt(priv_ep), corrupting the EP_STALLED flag of an
unrelated IN endpoint and potentially breaking communication for that
endpoint.

>  	priv_ep = priv_dev->eps[index];
>  
>  	cdns3_select_ep(priv_dev, le16_to_cpu(ctrl->wIndex));

[Severity: Medium]
Is it safe to pass the unvalidated, full 16-bit wIndex value down to the
hardware selector?

The USB host provides a 16-bit wIndex in the setup packet, which is only
implicitly masked by cdns3_ep_addr_to_index() when looking up the endpoint
index. This allows values with upper bits set (e.g., 0x0101) to pass the
array bounds check.

The raw, unmasked 16-bit wIndex is then passed to cdns3_select_ep(), which
casts it to u32 and writes it directly to the ep_sel hardware register.
Writing untrusted host-controlled data to reserved hardware register bits
can cause undefined device behavior or hardware state corruption.

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