Re: [PATCH v4 07/10] can: peak_usb: fix RX cursor desync for self-received RTR frames with SRR

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

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

- [Medium] This patch adds an error-path stats regression in
  pcan_usb_decode_data() (drivers/net/can/usb/peak_usb/pcan_usb.c).

--

can: peak_usb: fix RX cursor desync for self-received RTR frames with SRR

This commit moves the SRR private id byte skip in pcan_usb_decode_data()
out of the non-RTR branch. The byte is now consumed whenever
PCAN_USB_TX_SRR is set, which matches the layout used by
pcan_usb_encode_msg() and stops the RX cursor desync for self-received
RTR frames.

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
> index 93da0fd4f10e7..4c8cfed31634a 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
> @@ -697,16 +697,19 @@ static int pcan_usb_decode_data(struct pcan_usb_msg_context *mc, u8 status_len)
>  		memcpy(cf->data, mc->ptr, cf->len);
>  		mc->ptr += rec_len;
>  
> -		/* Ignore next byte (client private id) if SRR bit is set */
> -		if (can_id_flags & PCAN_USB_TX_SRR) {
> -			if ((mc->ptr + 1) > mc->end)
> -				goto decode_failed;
> -			mc->ptr++;
> -		}
> -
>  		/* update statistics */
>  		mc->netdev->stats.rx_bytes += cf->len;
>  	}
> +
> +	/* Ignore next byte (client private id) if SRR bit is set.
> +	 * Note: this test must be done even when the frame is an RTR frame.
> +	 */
> +	if (can_id_flags & PCAN_USB_TX_SRR) {
> +		if ((mc->ptr + 1) > mc->end)
> +			goto decode_failed;
> +		mc->ptr++;
> +	}
> +
>  	mc->netdev->stats.rx_packets++;

[Severity: Medium]
Can this leave rx_bytes counting a frame that gets dropped?

Before this patch, the SRR bounds check ran inside the non-RTR branch,
ahead of the rx_bytes update. Now rx_bytes is updated first, and the
check comes after it.

Take a non-RTR record that has PCAN_USB_TX_SRR set and a non-zero
payload ending exactly at mc->end, so the private id byte is missing.
In pcan_usb_decode_data():

    mc->ptr += rec_len;
    ...
    mc->netdev->stats.rx_bytes += cf->len;
    }
    ...
    if (can_id_flags & PCAN_USB_TX_SRR) {
        if ((mc->ptr + 1) > mc->end)
            goto decode_failed;

decode_failed frees the skb and returns -EINVAL. No frame is delivered
and rx_packets is not incremented, but the rx_bytes increment of up to
8 bytes is kept.

The device firmware controls this data, so a faulty or malicious USB
device could trigger it through:

peak_usb_read_bulk_callback()->pcan_usb_decode_buf()->
    pcan_usb_decode_msg()->pcan_usb_decode_data()

Would it work to do the non-RTR rx_bytes update after the shared SRR
check succeeds? For example, next to rx_packets++ and guarded by
!(status_len & PCAN_USB_STATUSLEN_RTR).

>  
>  	/* convert timestamp into kernel time */

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020-canfd_check_channel_idx-v4-0-bfbde81f8bab%40peak-system.fr