Re: [PATCH can-next 07/10] can: gs_usb: gs_usb_receive_bulk_callback(): report channel number out of range error

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message body of "can: gs_usb:
  gs_usb_receive_bulk_callback(): report channel number out of range…

Pre-existing issues:
- [High] In gs_usb_receive_bulk_callback()
  (drivers/net/can/usb/gs_usb.c), the channel check only tests the upper…

--

can: gs_usb: gs_usb_receive_bulk_callback(): report channel number out of range error

If the USB device sends a frame with an out of range channel number,
gs_usb_receive_bulk_callback() now logs a rate limited error before it
jumps to the device_detach label. Before this patch it detached without
logging anything. Nothing else changes.

> If the USB device sends an invalid channel, the device should not be
> silently detached; instead, the display an error message before detaching.

[Severity: Low]
This isn't a bug, but "instead, the display an error message" reads
wrong. Maybe "instead, display an error message before detaching"?

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 9a0635b9c2d44..85a10c43b6f73 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -627,8 +627,13 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
>  	}
>  
>  	/* device reports out of range channel id */
> -	if (hf->channel >= parent->channel_cnt)
> +	if (hf->channel >= parent->channel_cnt) {
> +		dev_err_ratelimited(&parent->udev->dev,
> +				    "channel number out of range (channel=%u, channel_cnt=%u)\n",
> +				    hf->channel, parent->channel_cnt);
> +
>  		goto device_detach;
> +	}
>  
>  	dev = parent->canch[hf->channel];

[Severity: High]
This isn't a bug introduced by this patch, but this check only tests the
upper bound. Can parent->canch[hf->channel] still be NULL when it is read
here?

The code right after this does:

	dev = parent->canch[hf->channel];

	netdev = dev->netdev;

gs_usb_probe() sets channel_cnt to cover all channels before any slot is
filled:

	parent->channel_cnt = icount;
	...
	for (i = 0; i < icount; i++) {
		...
		parent->canch[i] = gs_make_candev(i, intf, &dconf);

gs_make_candev() calls register_candev() before it returns. That means
can0 is visible to userspace while probe is still running the control
transfers for channel 1. Each transfer can take up to 1000 ms, and the
device decides how long it takes to answer.

If userspace brings can0 up in that window, gs_can_open() submits the RX
URBs:

	if (!parent->active_channels) {
		...

A device that reports icount >= 2 could then send a bulk-IN frame with
channel=1. The check above passes because 1 < channel_cnt. dev is then
NULL, and dev->netdev is dereferenced in URB completion context.

There is a related case when gs_make_candev() fails for a later channel.
The ERR_PTR is stored in the slot first:

	parent->canch[i] = gs_make_candev(i, intf, &dconf);
	if (IS_ERR_OR_NULL(parent->canch[i])) {

The rollback kills the RX URBs only when it destroys the earlier
channels. Until then, a frame for that channel would dereference the
ERR_PTR.

The device_detach loop has the same gap. Its test lets an ERR_PTR
through:

	if (parent->canch[rc])
		netif_device_detach(parent->canch[rc]->netdev);

Should this path also reject an empty slot, for example with
!parent->canch[hf->channel]? Another option is to stop storing ERR_PTR
values in canch[]. A third is to create all channels before any of them
is registered.

This check is still unchanged at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261004-gs_usb-cleanups-and-fixes-v1-0-7b4b06244a77%40pengutronix.de