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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] can: peak_usb: premature rx_bytes update in pcan_usb_decode_data()
--

commit c248d15b40b1ad16b34ab57a3fec82b4f7b5a805
Author: Stéphane Grosjean <s.grosjean@peak-system.fr>

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

This commit moves the PCAN_USB_TX_SRR client private ID byte skip out of the
non-RTR branch in pcan_usb_decode_data() so that self-received RTR frames
also advance the RX cursor past the trailing SRR byte.

> 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++;
> +	}

[Severity: Medium]
In drivers/net/can/usb/peak_usb/pcan_usb.c, pcan_usb_decode_data() now
increments mc->netdev->stats.rx_bytes inside the non-RTR branch before the
PCAN_USB_TX_SRR bounds check.

If a non-RTR frame has PCAN_USB_TX_SRR set in can_id_flags and the buffer is
truncated such that (mc->ptr + 1) > mc->end, this jumps to decode_failed:

	decode_failed:
		dev_kfree_skb(skb);
		return -EINVAL;

Doesn't this leave mc->netdev->stats.rx_bytes incremented for a malformed
frame that gets dropped and is not counted in rx_packets?

> +
>  	mc->netdev->stats.rx_packets++;
>  
>  	/* convert timestamp into kernel time */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-canfd_check_channel_idx-v5-0-a62b35b867f0@peak-system.fr?part=7