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

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

[James Gao]

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;
+
 	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;
+
 	struct peak_usb_device *dev = usb_if->dev[ctrl_idx];
 	struct net_device *netdev = dev->netdev;
 	struct can_frame *can_frame;
-- 
2.53.0

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

[Stéphane Grosjean]

Hi James,

Thanks for the fix.

I've reviewed the patch and it looks good to me.

Acked-by: Stéphane Grosjean <s.grosjean@peak-system.fr>

— Stéphane

________________________________________
De : James Gao <jamesgao5@outlook.com>
Envoyé : mercredi 13 mai 2026 11:51
À : mkl@pengutronix.de <mkl@pengutronix.de>; mailhol@kernel.org <mailhol@kernel.org>
Cc : linux-can@vger.kernel.org <linux-can@vger.kernel.org>; Stéphane Grosjean <s.grosjean@peak-system.fr>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; James Gao <jamesgao5@outlook.com>
Objet : [PATCH v2] can: peak_usb: Add bounds check for USB channel index
 
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;
+
         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;
+
         struct peak_usb_device *dev = usb_if->dev[ctrl_idx];
         struct net_device *netdev = dev->netdev;
         struct can_frame *can_frame;
--
2.53.0

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

[Vincent Mailhol]

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