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

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

Pre-existing issues:
- [Critical] Injecting a CAN XL frame causes a stack buffer overflow in `can327_handle_prompt()` due to an unvalidated `frame->len`.
- [High] Valid Classical CAN frames with a Data Length Code (DLC) greater than 8 are improperly rejected, leading to a state machine reset loop (DoS).
--

--- Patch [1]: [PATCH net v2] can: can327: Fix out-of-bounds write in can327_parse_frame() ---
commit dd9b0d69813d53aa1dc1be8d8a99a15956ba7c3d
Author: Baul Lee <[email protected]>

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

This patch fixes an out-of-bounds write in can327_parse_frame() by
validating the CAN payload length from the adapter. It adds checks to
reject frames if the DLC is not a valid hex digit or exceeds CAN_MAX_DLEN.

Link: https://lore.kernel.org/linux-can/[email protected]/

> diff --git a/drivers/net/can/can327.c b/drivers/net/can/can327.c
> index 90f5e35f3c8f9..c76a6378d4d62 100644
> --- a/drivers/net/can/can327.c
> +++ b/drivers/net/can/can327.c

[ ... ]

> @@ -460,7 +461,13 @@ static int can327_parse_frame(struct can327 *elm, size_t len)
>  	 */
>  
>  	/* Read CAN data length */
> -	frame->len = (hex_to_bin(elm->rxbuf[datastart - 2]) << 0);
> +	dlc = hex_to_bin(elm->rxbuf[datastart - 2]);
> +	if (dlc < 0 || dlc > CAN_MAX_DLEN) {
> +		/* Not a hex digit, or more than CAN_MAX_DLEN bytes. */
> +		kfree_skb(skb);
> +		return -ENODATA;
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but does returning -ENODATA for
frames with a DLC greater than 8 lead to a state machine reset loop?

Classical CAN frames can have a DLC between 9 and 15 on the bus. The ELM327
adapter outputs the raw DLC nibble. If this check fails and returns -ENODATA,
can327_parse_line() treats it as an unrecoverable error and calls
can327_kick_into_cmd_mode() to reset the adapter:

drivers/net/can/can327.c:can327_parse_line() {
    ...
    if (ret == -ENODATA) {
        /* Start afresh. */
        can327_kick_into_cmd_mode(elm);
    ...
}

Will this cause a denial of service by continuously resetting the CAN
interface whenever a valid frame with DLC > 8 is received?

> +	frame->len = dlc;
>  
>  	/* Read CAN ID */
>  	if (frame->can_id & CAN_EFF_FLAG) {

[Severity: Critical]
This is a pre-existing issue, but is there a stack buffer overflow in
can327_handle_prompt() when handling CAN XL frames?

If can_dev_dropped_skb() in include/linux/can/dev.h fails to drop ETH_P_CANXL
frames for devices lacking CAN XL support, an attacker could send a CAN XL
frame via a PF_PACKET or PF_CAN raw socket.

When can327_netdev_start_xmit() copies the first 16 bytes into
elm->can_frame_to_send, the CAN XL flags field aliases with frame->len,
setting it to 128 or more (since CANXL_XLF is 0x80).

In can327_handle_prompt(), the loop iterates over this attacker-controlled
length:

drivers/net/can/can327.c:can327_handle_prompt() {
    ...
    for (i = 0; i < frame->len; i++) {
        snprintf(&local_txbuf[2 * i],
                 sizeof(local_txbuf), "%02X",
                 frame->data[i]);
    }
    ...
}

Since sizeof(local_txbuf) is only 18, will this loop overflow local_txbuf[]
by writing hex bytes far past the end of the stack buffer, overwriting the
stack frame and return address?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1