Fwd: [PATCH 4/5] can: peak_usb: validate channel numbers in PCAN-USB FD

["Stéphane Grosjean" <stephane.grosjean@free.fr>]

Hello Marc, Vincent and Oliver,

I need help (or training) to figure out how to deal with these AI bots:

- How many of them are there before the changes are accepted? It seems that even if a patch has passed the Sashiko filter, we should still expect feedback from other bots...

- The fact that there are multiple levels of bots isn’t a problem in itself, and is understandable. That feedback arrives on v1 even though v2 has already been analysed is also something I’m just learning: so when can we consider it safe to push a new version without fear of receiving comments on the previous one?

- And finally, providing more explicit comments on our changes is also a good thing. But when those comments themselves become a source of problems and rejections—due to an overly nitpicky interpretation by a bot—it becomes far too time-consuming:

> The "entire message buffer is discarded" wording also reads as if
> this
> patch adds that behaviour. The goto fail path in
> pcan_usb_fd_decode_buf()
> is unchanged. Records decoded before the bad one have already been
> passed
> up via netif_rx(), and only the bad record and the ones after it are
> dropped.

Best regards,

-- Stéphane


----- Mail transféré -----
> De: netdev-bot+sashiko@kernel.org
> À: "stephane grosjean" <stephane.grosjean@free.fr>
> Cc: mkl@pengutronix.de, mailhol@kernel.org, error27@gmail.com, wg@grandegger.com, linux-can@vger.kernel.org,
> linux-kernel@vger.kernel.org, "s grosjean" <s.grosjean@peak-system.fr>, kuba@kernel.org
> Envoyé: Samedi 10 Octobre 2026 08:48:14
> Objet: Re: [PATCH 4/5] can: peak_usb: validate channel numbers in PCAN-USB FD
> 
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 1
> 
> If you believe none of the issues are legitimate, please reply to
> each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let
> patchwork
> know by including "pw-bot: cr" as a separate line at the end of your
> reply
> (one such reply per series is enough).
> 
> - [High] The new check `channel >= usb_if->adapter->ctrl_count` only
>   keeps the index inside dev[].
> - [Low] The commit message for "can: peak_usb: validate channel
> numbers
>   in PCAN-USB FD" says the patch "prevents potential out-of-bounds…
> 
> --
> 
> can: peak_usb: validate channel numbers in PCAN-USB FD
> 
> This patch changes the channel number check in the four PCAN-USB FD
> decoders. The old bound was ARRAY_SIZE(usb_if->dev). The new bound is
> the
> adapter's ctrl_count, read through an adapter pointer that is now
> stored in
> struct pcan_usb_fd_if. Invalid channels now return -EINVAL instead of
> -ENOMEM.
> 
> > Validate the channel number against the number of CAN controllers
> > supported by the adapter before performing the lookup. Any message
> > containing an invalid channel number is treated as malformed and
> > the
> > entire message buffer is discarded, as the device is considered to
> > be
> > providing untrusted data.
> 
> [ ... ]
> 
> > This prevents potential out-of-bounds accesses when handling
> > unexpected or corrupted messages received from PCAN-USB FD family
> > devices.
> 
> [Severity: Low]
> Is "out-of-bounds" the right description here? This was the old
> check:
> 
> 	if (pucan_msg_get_channel(rm) >= ARRAY_SIZE(usb_if->dev))
> 
> dev[] has PCAN_USB_MAX_CHANNEL (2) entries. No index outside
> dev[0..1]
> could get past that check before this patch.
> 
> What this patch seems to fix is a NULL pointer dereference on the
> single-channel adapters, PCAN-USB FD and PCAN-Chip USB. Both use
> .ctrl_count = PCAN_USBFD_CHANNEL_COUNT (1). pcan_usb_fd_init() only
> fills
> usb_if->dev[dev->ctrl_idx], and usb_if comes from kzalloc. On those
> adapters dev[1] therefore stays NULL.
> 
> Before this patch, a device record with channel 1 passed the "< 2"
> check.
> Then, in URB completion context:
> 
>   pcan_usb_fd_decode_canmsg(), _decode_status() and _decode_overrun()
>       read dev->netdev
>   pcan_usb_fd_decode_error()
>       wrote pdev->bec through container_of(NULL)
> 
> Could the commit message name this device-triggered NULL dereference
> and
> the affected adapters? There is no Cc: stable tag, so this matters
> for
> backport decisions.
> 
> The "entire message buffer is discarded" wording also reads as if
> this
> patch adds that behaviour. The goto fail path in
> pcan_usb_fd_decode_buf()
> is unchanged. Records decoded before the bad one have already been
> passed
> up via netif_rx(), and only the bad record and the ones after it are
> dropped.
> 
> On the -EINVAL change: pcan_usb_fd_decode_error() and
> pcan_usb_fd_decode_overrun() already returned -EINVAL before this
> patch.
> peak_usb_read_bulk_callback() also ignores the returned error code.
> 
> > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > index 82502594a4092..ddbb2be1de0dd 100644
> > --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> 
> [ ... ]
> 
> > @@ -536,8 +537,12 @@ static int pcan_usb_fd_decode_canmsg(struct
> > pcan_usb_fd_if *usb_if,
> >  	struct sk_buff *skb;
> >  	const u16 rx_msg_flags = le16_to_cpu(rm->flags);
> >  
> > -	if (pucan_msg_get_channel(rm) >= ARRAY_SIZE(usb_if->dev))
> > -		return -ENOMEM;
> > +	/* Reject invalid channel numbers reported by the firmware.
> > +	 * Any channel below ctrl_count is guaranteed to have a valid
> > +	 * entry in usb_if->dev[].
> > +	 */
> > +	if (pucan_msg_get_channel(rm) >= usb_if->adapter->ctrl_count)
> > +		return -EINVAL;
> >  
> >  	dev = usb_if->dev[pucan_msg_get_channel(rm)];
> >  	netdev = dev->netdev;
> 
> [Severity: High]
> Does this guarantee hold? The ctrl_count check keeps the index inside
> dev[], but nothing makes sure usb_if->dev[channel] points to a live
> device.
> 
> On the 2-channel adapters (PCAN-USB Pro FD and PCAN-USB X6,
> ctrl_count =
> PCAN_USBPROFD_CHANNEL_COUNT = 2), the new check is the same as the
> old
> ARRAY_SIZE() check.
> 
> pcan_usb_fd_decode_status(), pcan_usb_fd_decode_error() and
> pcan_usb_fd_decode_overrun() repeat the same comment and check.
> 
> The first case is a NULL entry during probe. peak_usb_probe() creates
> channels one at a time, and peak_usb_create_dev() registers the
> netdev
> before it calls dev_init():
> 
> peak_usb_create_dev()
>     register_candev(netdev);
>     ...
>     dev->adapter->dev_init(dev);
>         pcan_usb_fd_init()
>             pdev->usb_if->dev[dev->ctrl_idx] = dev;
> 
> Once channel 0 is registered and initialised, can0 can be brought up
> while
> dev[1] is still NULL. The window covers channel 0's dev_set_bus() and
> dev_get_can_channel_id() bulk commands, which the device can stall
> for up
> to 1000 ms each. It also covers register_candev() for can1.
> 
> Opening can0 submits RX URBs on the bulk-IN endpoint that both
> channels
> share. pcan_usb_fd_decode_buf() routes each record by its own channel
> field.
> 
> Could a channel-1 record then pass the "1 >= 2" test and dereference
> NULL
> through dev->netdev here? The same would apply to dev->can.state in
> pcan_usb_fd_decode_status() and to the write to pdev->bec in
> pcan_usb_fd_decode_error().
> 
> The second case is a dangling entry when probe fails. If channel 1's
> dev_set_bus() fails after dev_init(), peak_usb_create_dev() takes
> this
> path:
> 
> adap_dev_free:
> 	if (dev->adapter->dev_free)
> 		dev->adapter->dev_free(dev);
> 
> lbl_unregister_candev:
> 	unregister_candev(netdev);
> 
> pcan_usb_fd_free() does nothing for a device that has prev_siblings
> set,
> and it never clears usb_if->dev[1]:
> 
> static void pcan_usb_fd_free(struct peak_usb_device *dev)
> {
> 	/* last device: can free shared objects now */
> 	if (!dev->prev_siblings && !dev->next_siblings) {
> 
> dev1 is then freed. peak_usb_probe() calls peak_usb_disconnect()
> directly,
> without usb_disable_interface(), so can0's RX URBs are still live.
> 
> Can a channel-1 record arriving in that window reach freed memory?
> That
> includes the write in pcan_usb_fd_decode_error().
> 
> Disconnects started by the USB core (unplug, sysfs unbind, rmmod) do
> not
> seem to be affected. peak_usb_driver does not set soft_unbind, so
> usb_unbind_interface() kills the URBs before ->disconnect().
> 
> The follow-up commit "can: peak_usb: harden PCAN-USB message
> validation"
> only touches pcan_usb.c and does not cover this.
> 
> Would it be more complete to treat a NULL usb_if->dev[ch] as a
> malformed
> message in each FD decoder? That would also cover the single-channel
> case
> this patch targets.
> 
> Clearing the entry on per-channel teardown or failure would handle
> the
> dangling case. The comment would also need to be corrected.
> 
> [ ... ]
> 
> --
> Sashiko AI review ·
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-canfd_check_channel_idx-v1-0-0a3bb82f4e09%40peak-system.fr
>