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

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

[liuc63]

From: Liu Chao <liuc63@xiaopeng.com>

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

Besides the out-of-bounds index, eps[index] may also be NULL for
endpoints that are not enabled, as cdns3_init_eps() only populates
entries for endpoints enabled in USB_CAP3.

Add a bounds check against CDNS3_ENDPOINTS_MAX_COUNT together with a
NULL check 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>
Reviewed-by: Weibin Liu <liuwb@xiaopeng.com>
Signed-off-by: Liu Chao <liuc63@xiaopeng.com>
---
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

v1: https://lore.kernel.org/linux-usb/20260917103145.3047850-1-liuc63@xiaopeng.com/
---
 drivers/usb/cdns3/cdns3-ep0.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/cdns3/cdns3-ep0.c b/drivers/usb/cdns3/cdns3-ep0.c
index e29989d57..d26fa89b6 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;
 		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;
 	priv_ep = priv_dev->eps[index];
 
 	cdns3_select_ep(priv_dev, le16_to_cpu(ctrl->wIndex));
-- 
2.50.1

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

[Peter Chen (Qualcomm)]

On 26-10-12 19:01:07, liuc63 wrote:
> From: Liu Chao <liuc63@xiaopeng.com>
> 
> 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_req_ep0_get_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.
> 
> Besides the out-of-bounds index, eps[index] may also be NULL for
> endpoints that are not enabled, as cdns3_init_eps() only populates
> entries for endpoints enabled in USB_CAP3.
> 
> Add a bounds check against CDNS3_ENDPOINTS_MAX_COUNT together with a
> NULL check 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>
> Reviewed-by: Weibin Liu <liuwb@xiaopeng.com>
> Signed-off-by: Liu Chao <liuc63@xiaopeng.com>

Acked-by: Peter Chen <peter.chen@kernel.org>

Peter
> ---
> 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
> 
> v1: https://lore.kernel.org/linux-usb/20260917103145.3047850-1-liuc63@xiaopeng.com/
> ---
>  drivers/usb/cdns3/cdns3-ep0.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/cdns3/cdns3-ep0.c b/drivers/usb/cdns3/cdns3-ep0.c
> index e29989d57..d26fa89b6 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;
>  		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;
>  	priv_ep = priv_dev->eps[index];
>  
>  	cdns3_select_ep(priv_dev, le16_to_cpu(ctrl->wIndex));
> -- 
> 2.50.1
> 

-- 

Thanks,
Peter Chen

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

[Peter Chen (Qualcomm)]

On 26-10-12 19:01:07, liuc63 wrote:
> From: Liu Chao <liuc63@xiaopeng.com>
> 
> 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_req_ep0_get_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.
> 
> Besides the out-of-bounds index, eps[index] may also be NULL for
> endpoints that are not enabled, as cdns3_init_eps() only populates
> entries for endpoints enabled in USB_CAP3.
> 
> Add a bounds check against CDNS3_ENDPOINTS_MAX_COUNT together with a
> NULL check 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>
> Reviewed-by: Weibin Liu <liuwb@xiaopeng.com>
> Signed-off-by: Liu Chao <liuc63@xiaopeng.com>
> ---
> 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
> 
> v1: https://lore.kernel.org/linux-usb/20260917103145.3047850-1-liuc63@xiaopeng.com/
> ---
>  drivers/usb/cdns3/cdns3-ep0.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/cdns3/cdns3-ep0.c b/drivers/usb/cdns3/cdns3-ep0.c
> index e29989d57..d26fa89b6 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;

Oh, I have seen sashiko AI comments. You may use USB_ENDPOINT_NUMBER_MASK to get
a valid EP number, then get 'index' according to its direction (IN or OUT), at last,
judge if priv_dev->eps[index] is NULL.

Peter

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

-- 

Thanks,
Peter Chen