Re: [PATCH v4 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 · 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] The patch only partly achieves its second goal.

--

can: peak_usb: only dump received bytes on decode error

This commit limits the dump in peak_usb_read_bulk_callback() to the
urb->actual_length bytes the device actually sent, instead of the whole
kmalloc()'ed RX buffer. It also puts the dump behind net_ratelimit(),
because 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 ccfc26d3e5a6b..b86ea6f6b07c6 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 flood described in the commit message for
the FD and Pro adapters?

dev->adapter->dev_decode_buf() runs before this check. For the PCAN-USB
FD family (USB FD, Pro FD, X6, Chip) it is pcan_usb_fd_decode_buf(). For
PCAN-USB Pro it is pcan_usb_pro_decode_buf(). Both decoders already dump
the buffer on error, with 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 ends the same way.

The device fully controls the paths that set err here. In
pcan_usb_fd_decode_buf() they are a truncated header, an oversized
rx_msg->size ("got frag rec: should inc usb rx buf sze"), a short record
("got short rec") and sub-decoder failures.

In pcan_usb_pro_decode_buf() they are an unsupported data_type ("got
unsupported rec in usb msg:"), a record that runs past msg_end ("got
frag rec: should inc usb rx buf size") and handler errors.

Each of these paths also calls netdev_err() with no rate limit.

After each failure, peak_usb_read_bulk_callback() resubmits the bulk-IN
URB, so the cycle repeats:

peak_usb_read_bulk_callback()
    dev->adapter->dev_decode_buf()
        pcan_usb_fd_decode_buf() / pcan_usb_pro_decode_buf()
            netdev_err(...)
            pcan_dump_mem(...)    <- no rate limit
    if (err && net_ratelimit())
        pcan_dump_mem(...)        <- same buffer, dumped again
    usb_submit_urb(...)

On these adapters, can a device that fails every transfer still flood
the kernel log with up to rx_buffer_size bytes of hex per URB?

Only the original PCAN-USB adapter seems fully covered, since its decoder
in pcan_usb.c does not dump.

Should the adapter-level dumps in pcan_usb_fd.c and pcan_usb_pro.c also
be rate limited or removed? The other option would be to keep a single
rate-limited dump here in the core.

>  		}
>  	}
>

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