Re: [PATCH net] can: can327: Fix stack out-of-bounds write in can327_handle_prompt()

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The patch prevents a stack crash but results in transmitting an invalid CAN XL frame as garbage classical CAN data.

Pre-existing issues:
- [High] The patch addresses a local stack out-of-bounds write in can327 but fails to fix the subsystem-wide root cause: can_dev_dropped_skb() does not reject CAN XL frames for non-XL devices.
- [High] Sending a short (13-byte) CAN XL frame via an AF_PACKET RAW socket leaks 3 bytes of uninitialized kernel heap memory over UART.
- [High] Missing bounds check on parsed frame->len causes a heap array out-of-bounds write in can327_parse_frame().
--

--- Patch [1]: [PATCH net] can: can327: Fix stack out-of-bounds write in can327_handle_prompt() ---
commit 07e6406926056d156de760bd7d0a03a7acd9ad40
Author: Baul Lee <baul.lee@xbow.com>

can: can327: Fix stack out-of-bounds write in can327_handle_prompt()

This commit addresses a stack out-of-bounds write in can327_handle_prompt()
caused by an aliased CAN XL frame length. It clamps the hexdump loop to
CAN_MAX_DLEN to prevent writing past the 18-byte local_txbuf.

>    A CAN XL frame aliases struct can_frame such that canxl_frame.flags,
>    which must carry CANXL_XLF, overlaps can_frame.len, so it arrives with
>    frame->len == 0x80. can327 is a classical CAN device, but
>    can_dev_dropped_skb() only rejects CAN FD for a non-FD device; for
>    ETH_P_CANXL, can_dropped_invalid_skb() checks only that the frame is a
>    well-formed CAN XL frame, not that the device supports one.

[Severity: High]
This isn't a bug introduced by this patch, but does can_dev_dropped_skb() in
include/linux/can/dev.h need to drop CAN XL frames for non-XL hardware to
prevent a subsystem-wide issue?

Because canxl_frame.flags aliases with can_frame.len (resulting in a length
of 128), classical CAN drivers misinterpret the length. For example, does
this allow slcan_encaps() in drivers/net/can/slcan/slcan-core.c to hex-dump
128 bytes into a 33-byte heap buffer, or sja1000_start_xmit() in
drivers/net/can/sja1000/sja1000.c to write 128 out-of-bounds bytes to
hardware registers?

> diff --git a/drivers/net/can/can327.c b/drivers/net/can/can327.c
> index 90f5e35f3c8f9..396db56775a64 100644
> --- a/drivers/net/can/can327.c
> +++ b/drivers/net/can/can327.c
> @@ -624,9 +624,10 @@ static void can327_handle_prompt(struct can327 *elm)
>  			snprintf(local_txbuf, sizeof(local_txbuf), "ATRTR\r");
>  		} else {
>  			/* Send a regular CAN data frame */
> +			int dlc = min_t(int, frame->len, CAN_MAX_DLEN);

[Severity: Medium]
Instead of artificially clamping the length to 8, should we drop the
unsupported CAN XL frame entirely? 

By clamping it, it looks like we might transmit internal CAN XL headers as
classical CAN payload bytes. Does this cause a CAN XL frame to be
transmitted as garbage classical CAN data over the bus?

>  			int i;
>  
> -			for (i = 0; i < frame->len; i++) {
> +			for (i = 0; i < dlc; i++) {
>  				snprintf(&local_txbuf[2 * i],
>  					 sizeof(local_txbuf), "%02X",
>  					 frame->data[i]);

[Severity: High]
This is a pre-existing issue, but does sending a short (13-byte) CAN XL
frame via an AF_PACKET RAW socket leak uninitialized kernel heap memory
over UART?

In can327_netdev_start_xmit(), the driver passes a 13-byte SKB to
can327_send_frame(), which performs a 16-byte by-value struct copy:

can327_send_frame()
   elm->can_frame_to_send = *frame;

This captures 3 bytes of uninitialized heap memory. Then here in
can327_handle_prompt(), the loop formats offsets 8 through 15 of the
struct and queues them to be sent over the serial line.

[ ... ]

[Severity: High]
This is another pre-existing issue, but does can327_parse_frame() lack
bounds checking on the parsed frame->len?

If a malicious or faulty ELM327 device sends a malformed CAN data line
with a parsed length up to 15, frame->len is set directly:

drivers/net/can/can327.c:can327_parse_frame() {
    ...
    /* Read CAN data length */
    frame->len = (hex_to_bin(elm->rxbuf[datastart - 2]) << 0);
    ...
    /* Parse the data nibbles. */
    for (i = 0; i < frame->len; i++) {
        frame->data[i] =
            (hex_to_bin(elm->rxbuf[datastart + 3 * i]) << 4) |
            (hex_to_bin(elm->rxbuf[datastart + 3 * i + 1]));
    }
    ...
}

Since the skb is allocated via alloc_can_skb() (which allocates exactly
16 bytes for a classical CAN frame), does writing 15 bytes starting at
offset 8 corrupt adjacent heap memory like skb_shared_info?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819054201.67294-1-baul.lee@xbow.com?part=1