Re: [PATCH v6 11/13] can: peak_usb: don't read past the message in PCAN-USB Pro decoders

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message overstates what the old
  pcan_usb_pro_wait_rsp() bug could do.

--

can: peak_usb: don't read past the message in PCAN-USB Pro decoders

This commit adds bounds checks to pcan_usb_pro_decode_buf() and
pcan_usb_pro_wait_rsp(). With these checks, the record walk stops at the
end of the received message and no longer trusts the record count sent by
the device. pcan_usb_pro_wait_rsp() also now rejects a record that doesn't
fit in the received length.

> - pcan_usb_pro_wait_rsp() doesn't check the records against the
>   received length at all. Since the count is a 32-bit value, a bogus
>   response makes it walk the records past the command buffer.

[Severity: Low]
With the current callers, is the last sentence accurate? The code change
looks correct. It is less clear that the old walk could ever leave the
command buffer allocation.

pcan_usb_pro_get_can_channel_id() in
drivers/net/can/usb/peak_usb/pcan_usb_pro.c is the only caller of
pcan_usb_pro_wait_rsp(). It builds a 4-byte header plus an 8-byte
GETDEVID record in dev->cmd_buf:

pcan_usb_pro_get_can_channel_id() {
	...
	pc = pcan_msg_init_empty(&um, dev->cmd_buf, PCAN_USB_MAX_CMD_LEN);
	pcan_msg_add_rec(&um, PCAN_USBPRO_GETDEVID, dev->ctrl_idx);
	...
}

That makes um.rec_buffer_len 12. pcan_usb_pro_wait_rsp() passes that
length to usb_bulk_msg(), so the device can write at most 12 bytes.

peak_usb_create_dev() allocates dev->cmd_buf with
kzalloc(PCAN_USB_MAX_CMD_LEN), and PCAN_USB_MAX_CMD_LEN is 32. None of the
PCAN-USB Pro command builders write more than 12 bytes into it, so bytes
12-31 stay zero.

No record type 0x00 is defined, so pcan_usb_pro_sizeof_rec[0] is 0. All
record sizes are multiples of 4, and the largest is RXMSG8 at 20 bytes.

The old loop therefore reads type bytes the device controls only at
offsets 4 and 8. From offset 8 it can advance to offset 28 at most. There
it reads a zero type and stops on the !rec_len check, whatever value
rec_cnt has.

What the old code could do is read past actual_length. For example, it
could accept a truncated response with actual_length == 8 and a GETDEVID
record at offset 4. In that case it would return the stale request bytes
8-11 as dev_num. The new "pc + rec_len > rsp_end" check fixes that case.

Could the commit message describe this case instead, so that the stated
impact matches what the old pcan_usb_pro_wait_rsp() code could actually
do?

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