Re: [PATCH v2] can: peak_usb: Add bounds check for USB channel index

[Vincent Mailhol <mailhol@kernel.org>]

On 13/05/2026 at 11:51, James Gao wrote:
> The channel control index ctrl_idx is derived from rx->len which comes
> directly from a device USB payload. The mask 0x0f allows values 0-15,
> but the array size of usb_if->dev[] is only 2. Values 2-15 cause heap
> out-of-bounds read, eventually causing kernel panic in the IRQ context.
> 
> Add bounds checking for ctrl_idx before the array access in both
> pcan_usb_pro_handle_canmsg() and pcan_usb_pro_handle_error().
> 
> Fixes: d8a199355f8f ("can: usb: PEAK-System Technik PCAN-USB Pro specific part")
> Signed-off-by: James Gao <jamesgao5@outlook.com>
> ---
> 
> Hi Marc,
> 	Thanks for the review for v1. Since this is not a security bug I have
> 	moved the discussion to public mail list. Changes in v2:
> 	- Use ARRAY_SIZE(usb_if->dev) instead of PCAN_USBPRO_CHANNEL_COUNT
> 	- Use -EINVAL instead of -ENOMEM
> 	- Add newline between variable declaration and bounds check
> 	- Wrap commit message
> 
>  drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> index 4bfa8d0fbb32..5b1e3501cfb8 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> @@ -534,6 +534,10 @@ static int pcan_usb_pro_handle_canmsg(struct pcan_usb_pro_interface *usb_if,
>  				      struct pcan_usb_pro_rxmsg *rx)
>  {
>  	const unsigned int ctrl_idx = (rx->len >> 4) & 0x0f;
> +
> +	if (ctrl_idx >= ARRAY_SIZE(usb_if->dev))
> +		return -EINVAL;

Please don't tangle declarations and expressions.

	const unsigned int ctrl_idx = (rx->len >> 4) & 0x0f;
	struct peak_usb_device *dev;
	struct net_device *netdev;
	struct can_frame *can_frame;
	struct sk_buff *skb;
	struct skb_shared_hwtstamps *hwts;

	if (ctrl_idx >= ARRAY_SIZE(usb_if->dev))
		return -EINVAL;

	dev = usb_if->dev[ctrl_idx];
	netdev = dev->netdev;

Aside from that, looks OK.

>  	struct peak_usb_device *dev = usb_if->dev[ctrl_idx];
>  	struct net_device *netdev = dev->netdev;
>  	struct can_frame *can_frame;
> @@ -573,6 +577,10 @@ static int pcan_usb_pro_handle_error(struct pcan_usb_pro_interface *usb_if,
>  {
>  	const u16 raw_status = le16_to_cpu(er->status);
>  	const unsigned int ctrl_idx = (er->channel >> 4) & 0x0f;
> +
> +	if (ctrl_idx >= ARRAY_SIZE(usb_if->dev))
> +		return -EINVAL;

Same comment as above.

>  	struct peak_usb_device *dev = usb_if->dev[ctrl_idx];
>  	struct net_device *netdev = dev->netdev;
>  	struct can_frame *can_frame;


Yours sincerely,
Vincent Mailhol