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

[Peter Chen <peter.chen@kernel.org>]

On 26-09-17 18:31:45, Liu Chao wrote:
> cdns3_ep_addr_to_index() computes (ep_addr & 0x7F) + 16 for IN
> endpoints.  wIndex is supplied by the USB host, so ep_addr & 0x7F can
> be up to 127, giving an index up to 143.  The eps[] array has only
> CDNS3_ENDPOINTS_MAX_COUNT (32) entries.
> 
> Both cdns3_ep0_handle_status() (GET_STATUS) and
> cdns3_ep0_feature_handle_endpoint() (SET/CLEAR_FEATURE) pass the
> unvalidated index straight into priv_dev->eps[index] and then
> dereference the result.
> 
> This is the same class of vulnerability that was fixed in renesas_usb3
> by commit ee0d382feb44 (CVE-2026-31615).

Commit ee0d382feb44 is for aspeed_udc, but not renesas_usb3.
Also, if citing commit, it is better using format: commit <sha> ("<title>"),
do you run chechpatch.pl before submitting?

Besides, where the user could find what is CVE-2026-31615?

> 
> Add a bounds check against CDNS3_ENDPOINTS_MAX_COUNT in both functions.
> 
> Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
> Cc: stable@vger.kernel.org
> Reviewed-by: Weibin Liu <liuwb@xiaopeng.com>
> Signed-off-by: Liu Chao <liuc63@xiaopeng.com>
> ---
>  drivers/usb/cdns3/cdns3-ep0.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/cdns3/cdns3-ep0.c b/drivers/usb/cdns3/cdns3-ep0.c
> index e29989d57..76642eaaf 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 (index >= CDNS3_ENDPOINTS_MAX_COUNT)
> +			return -EINVAL;

Good caught. And it also needs to add one more condition for !priv_dev->eps[index] since
it may be NULL if endpoints are not enabled at usb_caps, see cdns3_init_eps() for detail.

Peter

>  		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 (index >= CDNS3_ENDPOINTS_MAX_COUNT)
> +		return -EINVAL;
>  	priv_ep = priv_dev->eps[index];
>  
>  	cdns3_select_ep(priv_dev, le16_to_cpu(ctrl->wIndex));
> -- 
> 2.50.1
> 

-- 

Thanks,
Peter Chen