Re: [PATCH v2 5/5] can: peak_usb: harden PCAN-USB message validation

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 (2) · 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 says it rejects PCAN-USB messages that would cause
  reads outside the received USB data, but one path is still unchecked:…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the encoder
  and decoder disagree on the wire layout for RTR frames with SRR set.
- [Medium] This problem wasn't introduced by this patch, but when the
  decoder rejects a message, the diagnostic dump prints the whole rx…

--

can: peak_usb: harden PCAN-USB message validation

This patch adds bounds checks to the PCAN-USB rx decoder. The status_len
byte at the start of each record and the SRR private-id byte after a data
frame are now consumed only if they fit inside the received USB buffer.
Malformed messages are rejected with -EINVAL.

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
> index 169c00d93463c..56ddd134bae0f 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
> @@ -696,8 +696,11 @@ static int pcan_usb_decode_data(struct pcan_usb_msg_context *mc, u8 status_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 (can_id_flags & PCAN_USB_TX_SRR) {
> +			if ((mc->ptr + 1) > mc->end)
> +				goto decode_failed;
>  			mc->ptr++;
> +		}

[Severity: Medium]
This is a pre-existing issue, but is the private-id byte also present for
RTR frames?

In pcan_usb_decode_data(), this skip and the new check exist only in the
non-RTR branch:

	if (status_len & PCAN_USB_STATUSLEN_RTR) {
		cf->can_id |= CAN_RTR_FLAG;
	} else {
		...
		if (can_id_flags & PCAN_USB_TX_SRR) {

But pcan_usb_encode_msg() appends the writer id for RTR frames as well:

	/* can data */
	if (!(cf->can_id & CAN_RTR_FLAG)) {
		memcpy(pc, cf->data, cf->len);
		pc += cf->len;
	}

	/* SRR bit needs a writer id (useless here) */
	if (can_id_flags & PCAN_USB_TX_SRR)
		*pc++ = 0x80;

SRR is set whenever CAN_CTRLMODE_LOOPBACK is enabled. Suppose the firmware's
loopback echo of an RTR frame includes that byte. Then mc->ptr is left
pointing at it, and pcan_usb_decode_msg() reads 0x80 as the next status_len
byte.

PCAN_USB_STATUSLEN_INTERNAL is clear in 0x80, so the byte would be decoded
as a bogus standard-ID data frame. Every later record in the URB would then
be parsed out of phase. That gives phantom frames, or an early -EINVAL that
drops the rest of the URB.

The real RTR echo format can't be confirmed from the source. Both the
encoder and decoder lines came from the same commit 3a7939495ce8, so the
difference may be intentional. The later reads are bounds-checked, so no
out-of-bounds access results.

Could the firmware behaviour be checked either way?

> @@ -733,14 +736,19 @@ static int pcan_usb_decode_msg(struct peak_usb_device *dev, u8 *ibuf, u32 lbuf)
>  	int err;
>  
>  	for (err = 0; mc.rec_idx < mc.rec_cnt && !err; mc.rec_idx++) {
> -		u8 sl = *mc.ptr++;
> -
> -		/* handle status and error frames here */
> -		if (sl & PCAN_USB_STATUSLEN_INTERNAL) {
> -			err = pcan_usb_decode_status(&mc, sl);
> -		/* handle normal can frames here */
> +		/* check if status_len byte can be read next */
> +		if (mc.ptr >= mc.end) {
> +			err = -EINVAL;

[Severity: Medium]
This isn't a bug introduced by this patch, but this new -EINVAL path (and
the truncated SRR one above) leads to the diagnostic dump in
peak_usb_read_bulk_callback():

	err = dev->adapter->dev_decode_buf(dev, urb);
	if (err)
		pcan_dump_mem("received usb message",
			      urb->transfer_buffer,
			      urb->transfer_buffer_length);

Doesn't this dump the full rx_buffer_size allocation instead of only the
urb->actual_length bytes the device sent?

The rx buffers come from kmalloc() in peak_usb_start() and are not zeroed.
After a short malformed transfer such as [00 01 00] (truncated CAN id), the
bytes past the received data are uninitialized slab contents.

pcan_dump_mem() prints them at KERN_INFO with print_hex_dump(), with no rate
limit and no debug-only guard. Earlier decode failures could already reach
this dump.

Would it make sense to pass urb->actual_length here instead?

>  		} else {
> -			err = pcan_usb_decode_data(&mc, sl);
> +			u8 sl = *mc.ptr++;
> +
> +			/* handle status and error frames here */
> +			if (sl & PCAN_USB_STATUSLEN_INTERNAL) {
> +				err = pcan_usb_decode_status(&mc, sl);

[Severity: High]
Can a truncated bus event record still cause an out-of-bounds read here?

pcan_usb_decode_status() only checks that the function and number bytes
fit, and then advances past them:

	if ((mc->ptr + 2) > mc->end)
		return -EINVAL;

	f = mc->ptr[PCAN_USB_CMD_FUNC];
	n = mc->ptr[PCAN_USB_CMD_NUM];
	mc->ptr += PCAN_USB_CMD_ARGS;

At this point mc->ptr can equal mc->end. Suppose the timestamp bit is clear,
f is PCAN_USB_REC_BUSEVT, and n is PCAN_USB_ERR_CNT_DEC or
PCAN_USB_ERR_CNT_INC. Then the call goes straight to
pcan_usb_handle_bus_evt():

		pdev->bec.rxerr = mc->ptr[1];
		pdev->bec.txerr = mc->ptr[2];

These read mc->end + 1 and mc->end + 2 with no check. The later
(mc->ptr + rec_len) > mc->end check in pcan_usb_decode_status() runs only
after these reads. rec_len also comes from the device and can be 0.

For example, the 5 byte transfer [00 01 40 05 00] passes the new status_len
check. It then reads ibuf[6] and ibuf[7], past actual_length, and is
reported as successfully decoded.

Now take a full 64 byte transfer whose last record is a bus event header
ending at the last byte. The reads land on ibuf[65] and ibuf[66], which are
outside the kmalloc-64 rx buffer allocated in peak_usb_start().

Those bytes are stored in pdev->bec. Userspace can read them through
pcan_usb_get_berr_counter() (IFLA_CAN_BERR_COUNTER) and in the CAN error
frames built by pcan_usb_decode_error().

The commit message says the patch rejects buffers "whose contents would
otherwise lead to accesses outside the received USB data area". Should
pcan_usb_handle_bus_evt() check something like (mc->ptr + 3) > mc->end
before reading these bytes, rather than relying on rec_len from the device?

>  			/* handle normal can frames here */
> +			} else {
> +				err = pcan_usb_decode_data(&mc, sl);
> +			}
>  		}
>  	}

[ ... ]

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