Re: [PATCH v5 10/10] can: peak_usb: only dump received bytes on decode error

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 (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).

- [High] Incomplete fix: the patch does not meet its stated rate-limiting
  goal for most of the PEAK adapters.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  pcan_usb_pro_decode_buf() reads pr->data_type at rec_ptr before…

--

can: peak_usb: only dump received bytes on decode error

When decoding a received URB fails, peak_usb_read_bulk_callback() now
dumps only urb->actual_length bytes instead of the whole RX buffer. The
dump is also rate limited, since a faulty or malicious device can make
every transfer fail to decode.

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> index a12ee6ec71e68..e146c8940420a 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> @@ -265,10 +265,10 @@ static void peak_usb_read_bulk_callback(struct urb *urb)
>  		/* handle these kinds of msgs only if _start callback called */
>  		if (test_bit(PCAN_USB_STATE_STARTED, &dev->state)) {
>  			err = dev->adapter->dev_decode_buf(dev, urb);
> -			if (err)
> +			if (err && net_ratelimit())
>  				pcan_dump_mem("received usb message",
>  					      urb->transfer_buffer,
> -					      urb->transfer_buffer_length);
> +					      urb->actual_length);

[Severity: High]
Does this rate limit cover the FD and Pro adapters? The commit message
says:

  Since a faulty or malicious device can make every transfer fail to
  decode, also rate limit the dump.

For the PCAN-USB FD family and for PCAN-USB Pro, dev_decode_buf() has
already dumped the same bytes before it returns the error, and that dump
has no rate limit:

drivers/net/can/usb/peak_usb/pcan_usb_fd.c:pcan_usb_fd_decode_buf() {
    ...
fail:
	if (err)
		pcan_dump_mem("received msg",
			      urb->transfer_buffer, urb->actual_length);
	return err;
}

pcan_usb_pro_decode_buf() in pcan_usb_pro.c has the same fail: block.

The device decides whether those paths run:

- In FD, a "got frag rec" or "got short rec" record returns -EBADMSG.
- In Pro, an unknown data_type returns -ENOTSUPP, and a record that
  crosses msg_end returns -EBADMSG.

Each of those paths also prints a netdev_err() with no rate limit.

A device that fails every URB would therefore still flood the log with
one full hex dump per URB (up to 2048 bytes for FD), plus the
netdev_err(). On top of that, the core dump in this hunk prints the same
buffer a second time whenever net_ratelimit() allows it.

Only the original PCAN-USB adapter (pcan_usb.c) gets the intended
behaviour, because its decoder does not dump anything itself.

Should the adapter-level dumps and their netdev_err() calls also be rate
limited? Or could the core dump be skipped for adapters that already
dump? Either way, each decode error would be dumped at most once and
always under a rate limit.

[Severity: Medium]
This isn't a bug introduced by this patch, but I noticed it in the Pro
decode path. pcan_usb_pro_decode_buf() in pcan_usb_pro.c reads
pr->data_type before it checks rec_ptr against msg_end:

	for (; rec_cnt > 0; rec_cnt--) {
		union pcan_usb_pro_rec *pr = (union pcan_usb_pro_rec *)rec_ptr;
		u16 sizeof_rec = pcan_usb_pro_sizeof_rec[pr->data_type];
		...
		/* check if the record goes out of current packet */
		if (rec_ptr + sizeof_rec > msg_end) {

The device supplies rec_cnt. If a record ends exactly at msg_end and
rec_cnt is still non-zero, the next iteration reads *msg_end.

The RX buffer is kmalloc(PCAN_USBPRO_RX_BUFFER_SIZE), which is 1024
bytes. Take a 1024-byte transfer with a 4-byte header, rec_cnt = 86, and
85 RXTS records of 12 bytes each. The 86th iteration then reads
transfer_buffer[1024], one byte past the slab object. For shorter
transfers it reads stale bytes left in the buffer.

The byte only decides between -ENOTSUPP and -EBADMSG. Still, would KASAN
report this read as slab-out-of-bounds?

The FD decoder avoids this by checking msg_end - msg_ptr against
sizeof(*rx_msg) at the top of its loop. Could pcan_usb_pro_decode_buf()
use a similar rec_ptr >= msg_end check before the dereference?

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