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

[sashiko-bot@kernel.org]

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