Message-ID: <84998b1d-8b3e-4956-b7fd-323e4999dc7c@wanadoo.fr>
Date: Tue, 19 Nov 2024 16:41:57 +0900
From: Vincent Mailhol <mailhol.vincent@...adoo.fr>
To: Max Staudt <max@...as.org>, Dan Carpenter <dan.carpenter@...aro.org>,
 Marc Kleine-Budde <mkl@...gutronix.de>
Cc: linux-can@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] can: can327: Clean up payload encoding in
 can327_handle_prompt()

On 19/11/2024 at 09:38, Max Staudt wrote:
> The hex dump encoding of outgoing payloads used snprintf() with a
> too large length of the target buffer. While the length was wrong, the
> buffer size and its filling logic were fine (sprintf() would have been
> sufficient), hence this is not security relevant.
> 
> Still, it's a good opportunity to simplify the code, and since no
> length checking is required, let's implement it with bin2hex().
> 
> Since bin2hex() outputs lowercase letters, this changes the spoken
> wire protocol with the ELM327 chip, resulting in a change in
> can327_is_valid_rx_char() because the ELM327 is set to echo the
> characters sent to it. The documentation says that this is fine, and
> I have verified the change on actual hardware.

Nice that the device accepts the lower case hexadecimals. This results
in a nice simplification.

> Finally, since the reporter's worry was that frame->len may be larger
> than 8, resulting in a buffer overflow in can327_handle_prompt()'s
> local_txbuf, a comment describes how the CAN stack prevents that. This
> is also the reason why the size passed to snprintf() was not relevant
> to preventing a buffer overflow, because there was no overflow possible
> in the first place.
> 
> Fixes: 43da2f07622f ("can: can327: CAN/ldisc driver for ELM327 based OBD-II adapters")
> Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
> Tested-by: Max Staudt <max@...as.org>
> Signed-off-by: Max Staudt <max@...as.org>

Reviewed-by: Vincent Mailhol <mailhol.vincent@...adoo.fr>

I left comments on the comments. If you have time, it would be wonderful
if your comment on start_xmit() could be moved to can_dev_dropped_skb()
in a separate patch.

The code is good, so if such rework is not feasible, I am happy to take
it as-is.

> ---
>  drivers/net/can/can327.c | 46 ++++++++++++++++++++++++++++------------
>  1 file changed, 33 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/can/can327.c b/drivers/net/can/can327.c
> index 24af63961030..3ae7b4eb6ca5 100644
> --- a/drivers/net/can/can327.c
> +++ b/drivers/net/can/can327.c
> @@ -18,6 +18,7 @@
>  #include <linux/bitops.h>
>  #include <linux/ctype.h>
>  #include <linux/errno.h>
> +#include <linux/hex.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/lockdep.h>
> @@ -622,17 +623,14 @@ static void can327_handle_prompt(struct can327 *elm)
>  			 */
>  			snprintf(local_txbuf, sizeof(local_txbuf), "ATRTR\r");
>  		} else {
> -			/* Send a regular CAN data frame */
> -			int i;
> -
> -			for (i = 0; i < frame->len; i++) {
> -				snprintf(&local_txbuf[2 * i],
> -					 sizeof(local_txbuf), "%02X",
> -					 frame->data[i]);
> -			}
> -
> -			snprintf(&local_txbuf[2 * i], sizeof(local_txbuf),
> -				 "\r");
> +			/* Send a regular CAN data frame.
> +			 *
> +			 * frame->len is guaranteed to be <= 8. Please refer
> +			 * to the comment in can327_netdev_start_xmit().
> +			 */

Nitpick, could be less verbose, e.g.:

  /* frame->len <= 8 enforced in can327_netdev_start_xmit() */

> +			bin2hex(local_txbuf, frame->data, frame->len);
> +			local_txbuf[2 * frame->len] = '\r';
> +			local_txbuf[2 * frame->len + 1] = '\0';
>  		}
>  
>  		elm->drop_next_line = 1;
> @@ -815,6 +813,26 @@ static netdev_tx_t can327_netdev_start_xmit(struct sk_buff *skb,
>  	struct can327 *elm = netdev_priv(dev);
>  	struct can_frame *frame = (struct can_frame *)skb->data;
>  
> +	/* Why this driver can rely on frame->len <= 8:
> +	 *
> +	 * While can_dev_dropped_skb() sanity checks the skb to contain a
> +	 * CAN 2.0, CAN FD, or other CAN frame type supported by the CAN
> +	 * stack, it does not restrict these types of CAN frames.
> +	 *
> +	 * Instead, this driver is guaranteed to receive only classic CAN 2.0
> +	 * frames, with frame->len <= 8, by a chain of checks around the CAN
> +	 * device's MTU (as of v6.12):
> +	 *
> +	 *  - can_changelink() sets the CAN device's MTU to CAN_MTU since we
> +	 *    don't advertise CAN_CTRLMODE_FD support in ctrlmode_supported.
> +	 *  - can_send() then refuses to pass any skb that exceeds CAN_MTU.
> +	 *  - Since CAN_MTU is the smallest currently (v6.12) supported CAN
> +	 *    MTU, it is clear that we are dealing with an ETH_P_CAN frame.
> +	 *  - All ETH_P_CAN (classic CAN 2.0) frames have frame->len <= 8,
> +	 *    as enforced by a call to can_is_can_skb() in can_send().
> +	 *  - Thus for all CAN frames reaching this function, frame->len <= 8.
> +	 */

Actually, none of this is really specific to your can327 driver.

While this is a valuable piece of information, I would rather like to
see this added as a kdoc comment on top of can_dev_dropped_skb(). That
function already has a one line documentation, but maybe it deserves a
longer paragraph?

One of the key point is that the userland is able to bypass the CAN_RAW
layer (or any other CAN layers) by sending AF_PACKET. In which case, the
packet directly reaches the driver without any prior sanitization. So it
is critical to highlight that drivers must call can_dev_dropped_skb() at
the top of their start_xmit() function, typically to avoid buffer
overflows because of an out of band frame->len.

>  	if (can_dev_dropped_skb(dev, skb))
>  		return NETDEV_TX_OK;
>  
> @@ -871,8 +889,10 @@ static bool can327_is_valid_rx_char(u8 c)
>  		['H'] = true, true, true, true, true, true, true,
>  		['O'] = true, true, true, true, true, true, true,
>  		['V'] = true, true, true, true, true,
> -		['a'] = true,
> -		['b'] = true,
> +		/* Note: c-f are needed only if outgoing CAN payloads are
> +		 * sent as lowercase hex dumps instead of uppercase.
> +		 */
> +		['a'] = true, true, true, true, true, true,
>  		['v'] = true,
>  		[CAN327_DUMMY_CHAR] = true,
>  	};

Yours sincerely,
Vincent Mailhol

Powered by blists - more mailing lists