Re: [PATCH net] can: peak_usb: fix slab-out-of-bounds read in pcan_usb_handle_bus_evt()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The main message decoding loop in `pcan_usb_decode_msg()` and the data decoding in `pcan_usb_decode_data()` lack proper bounds checks, leading to slab-out-of-bounds reads on maliciously crafted or malformed USB packets.
- [High] The decoding loop in `pcan_usb_pro_decode_buf()` is vulnerable to an out-of-bounds read because it dereferences `rec_ptr` before verifying that the pointer is within the buffer boundaries.
- [High] The `restart_timer` and unanchored restart URB are not cancelled during device disconnect, leading to a Use-After-Free.
- [High] Uninitialized kernel stack memory is sent to the USB device in `pcan_usb_set_bittiming` and `pcan_usb_set_can_channel_id`.
- [High] Casting a byte array on the stack to `__le32 *` can cause unaligned memory access exceptions on some architectures.
--

commit 5da73d58a5b6f6aab2dab8be2879ad21991bb88d
Author: Xiang Mei <xmei5@asu.edu>

can: peak_usb: fix slab-out-of-bounds read in pcan_usb_handle_bus_evt()

This commit fixes a slab-out-of-bounds read in pcan_usb_handle_bus_evt() by
checking that mc->ptr + 3 does not exceed mc->end before accessing the rxerr
and txerr counters.

Closes: https://lore.kernel.org/linux-can/upsCxxaJjg2FJWhfCx6KjFXLlh8o4IhaHKJi@bugs.sh/

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
> index 8fd058c328564..e87ed7657cc1b 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
> @@ -558,6 +558,8 @@ static int pcan_usb_handle_bus_evt(struct pcan_usb_msg_context *mc, u8 ir)
>  	switch (ir) {
>  	case PCAN_USB_ERR_CNT_DEC:
>  	case PCAN_USB_ERR_CNT_INC:
> +		if ((mc->ptr + 3) > mc->end)
> +			return -EINVAL;
>  
>  		/* save rx/tx error counters from in the device context */
>  		pdev->bec.rxerr = mc->ptr[1];

While reviewing this patch, I noticed some other pre-existing issues in this
driver.

This is a pre-existing issue, but does pcan_usb_decode_msg() also lack proper
bounds checks when reading from mc.ptr?

drivers/net/can/usb/peak_usb/pcan_usb.c:pcan_usb_decode_msg() {
	...
	for (err = 0; mc.rec_idx < mc.rec_cnt && !err; mc.rec_idx++) {
		u8 sl = *mc.ptr++;
	...
}

[Severity: High]
Does this code dereference mc.ptr without verifying that it has not exceeded
mc.end? If a malicious USB device sends a packet with rec_cnt set larger than
the available data, could this lead to a slab-out-of-bounds read?

Similarly, in pcan_usb_decode_data():

drivers/net/can/usb/peak_usb/pcan_usb.c:pcan_usb_decode_data() {
	...
		/* Ignore next byte (client private id) if SRR bit is set */
		if (can_id_flags & PCAN_USB_TX_SRR)
			mc->ptr++;
	...
}

[Severity: High]
Can this pointer increment bypass previous bounds checks and push the pointer
past the buffer's end for the next iteration?

This is a pre-existing issue, but does pcan_usb_pro_decode_buf() have a
similar out-of-bounds read vulnerability?

drivers/net/can/usb/peak_usb/pcan_usb_pro.c:pcan_usb_pro_decode_buf() {
	...
	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];
	...
}

[Severity: High]
Could this read the data_type field from memory before checking if rec_ptr is
within msg_end? If previous records exactly consume the buffer, could this
read past the end of the buffer?

This is a pre-existing issue, but is the restart_timer and unanchored restart
URB properly cancelled during device disconnect?

drivers/net/can/usb/peak_usb/pcan_usb.c:pcan_usb_restart_async() {
	...
	return usb_submit_urb(urb, GFP_ATOMIC);
}

drivers/net/can/usb/peak_usb/pcan_usb.c:pcan_usb_restart_pending() {
	...
	mod_timer(&pdev->restart_timer,
			jiffies + msecs_to_jiffies(PCAN_USB_STARTUP_TIMEOUT));
	...
}

drivers/net/can/usb/peak_usb/pcan_usb.c:pcan_usb_restart() {
	...
	struct pcan_usb *pdev = timer_container_of(pdev, t, restart_timer);
	...
}

[Severity: High]
Does this unanchored URB and uncancelled timer lead to a Use-After-Free if
the device is disconnected? Since the URB is not anchored to
dev->tx_submitted, the disconnect path won't kill it, and the timer could
fire and access freed memory.

This is a pre-existing issue, but do pcan_usb_set_bittiming() and
pcan_usb_set_can_channel_id() leak uninitialized stack memory to the USB
device?

drivers/net/can/usb/peak_usb/pcan_usb.c:pcan_usb_set_bittiming() {
	u8 args[PCAN_USB_CMD_ARGS_LEN];
	...
	args[0] = btr1;
	args[1] = btr0;

	return pcan_usb_send_cmd(dev, PCAN_USB_CMD_BITRATE, PCAN_USB_SET, args);
}

drivers/net/can/usb/peak_usb/pcan_usb.c:pcan_usb_set_can_channel_id() {
	u8 args[PCAN_USB_CMD_ARGS_LEN];
	...
	args[0] = can_ch_id;
	return pcan_usb_send_cmd(dev, PCAN_USB_CMD_DEVID, PCAN_USB_SET, args);
}

[Severity: High]
Are these args arrays passed to pcan_usb_send_cmd() without being fully
zero-initialized? Could this leak up to 13 bytes of uninitialized kernel
stack memory per command?

This is a pre-existing issue, but does pcan_usb_get_serial() cast an
unaligned byte array to a 4-byte integer pointer?

drivers/net/can/usb/peak_usb/pcan_usb.c:pcan_usb_get_serial() {
	u8 args[PCAN_USB_CMD_ARGS_LEN];
	...
	err = pcan_usb_wait_rsp(dev, PCAN_USB_CMD_SN, PCAN_USB_GET, args);
	if (err)
		return err;
	*serial_number = le32_to_cpup((__le32 *)args);
	...
}

[Severity: High]
Does casting this 1-byte aligned stack array to an unaligned __le32 pointer
cause alignment traps on architectures with strict alignment requirements?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906222132.3850994-1-xmei5@asu.edu?part=1