* Re: [PATCH net] can: peak_usb: fix slab-out-of-bounds read in pcan_usb_handle_bus_evt()
2026-09-06 22:21 [PATCH net] can: peak_usb: fix slab-out-of-bounds read in pcan_usb_handle_bus_evt() Xiang Mei
@ 2026-09-06 22:36 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-09-06 22:36 UTC (permalink / raw)
To: Xiang Mei
Cc: Oleksij Rempel, Marc Kleine-Budde, linux-can, Vincent Mailhol,
Oliver Hartkopp
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
^ permalink raw reply [flat|nested] 2+ messages in thread