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.

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@linaro.org>
Tested-by: Max Staudt <max@enpas.org>
Signed-off-by: Max Staudt <max@enpas.org>
---
 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().
+			 */
+			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.
+	 */
+
 	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,
 	};
-- 
2.39.5

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@linaro.org>
> Tested-by: Max Staudt <max@enpas.org>
> Signed-off-by: Max Staudt <max@enpas.org>

Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.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

On 11/19/24 16:41, Vincent Mailhol wrote:
> Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.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.

Thanks for the review! Yes, if you are fine with taking it, I suggest 
doing so now, and if I ever get to rework the comments, then I can look 
into this as well.


The thing is, when writing this patch, I tried to put myself in Dan's 
shoes. That is, someone unfamiliar with the code who is now looking at 
it. There's some string manipulation going on (uh-oh!), and we need to 
ensure that no buffers are overrun. Naturally, as a reviewer, I'd like 
to know that indeed, frame->len <= 8. Hence why I added a comment to 
that effect.

But security is not about trusting me, it's about proof. That's where 
the comment in _xmit() comes in. And I have to say, while it's easy to 
see that can_dev_dropped_skb() ensures len <= 8 in case of ETH_P_CAN, 
the guarantee that we only receive ETH_P_CAN packets in the first place 
is a whole different story. The MTU checks took a while for me to track 
down and understand, so my intention is to leave some breadcrumbs for 
the next poor soul digging into can327. Frankly, I think that's a sign 
that the otherwise lean CAN stack has finally accumulated some tech debt 
(or maybe I'm just bad at reading code), but oh well.

Now, as you say, this is sort of valid for all CAN drivers, and hence it 
should be in some centralised documentation. Agreed. But then again, for 
this driver, I wanted to make a point that we're dealing with ETH_P_CAN 
only, so the comment is also written that way. If you really don't want 
that there, I can shorten it and move it, but if it's okay as-is, then 
let's leave it in, and at a later stage, we can use it as a template for 
some more generic documentation :)


>> -			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() */

IMHO that's a matter of point of view - for you it may be obvious that 
the CAN stack only passes ETH_P_CAN to _xmit(), but to someone new it is 
not. And this particular detail is *not* enforced in _xmit(), but in 
can_send() in the CAN stack. I hope you can understand my wish to be 
precise about this comment, in order to avoid confusion when inevitably 
someone else comes looking for overflows. Or, well, if I want to touch 
can327 again in six months' time, when I have forgotten about all of this :)


>> +	/* 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?

Agreed that the can_dev_dropped_skb() documentation should be improved. 
However, I wouldn't remove the comment in can327 entirely - see above.


> 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.

Agreed, and that's exactly the rabbit hole I went down yesterday. Where 
would be the best place to document this? I guess that could go, er... 
in the CAN driver writer's guide? Do we have something like that?



A bit off-topic, but since CAN_RAW came up: Why does CAN_RAW even 
sanitise anything at all, instead of relying on checks on later layers? 
It seems like quite a few checks are duplicated. And all the while it's 
possible for userspace to do weird stuff like seemingly enabling CAN FD 
on the socket via setsockopt() CAN_RAW_FD_FRAMES, but that's only 
flipping a bit on the CAN_RAW socket, yet changes nothing underneath. It 
was quite confusing to read. I suppose the explanation is "legacy"?



Thanks for your review!

Max

On 11/20/24 02:15, Max Staudt wrote:
> A bit off-topic, but since CAN_RAW came up: Why does CAN_RAW even 
> sanitise anything at all, instead of relying on checks on later layers? 
> It seems like quite a few checks are duplicated. And all the while it's 
> possible for userspace to do weird stuff like seemingly enabling CAN FD 
> on the socket via setsockopt() CAN_RAW_FD_FRAMES, but that's only 
> flipping a bit on the CAN_RAW socket, yet changes nothing underneath. It 
> was quite confusing to read. I suppose the explanation is "legacy"?

Answering my own question here:

CAN_RAW_FD_FRAMES modifies what frames are allowed to pass through the 
CAN_RAW socket towards userspace. This way, simple software handling 
only CAN 2.0 frames may assume that reading anything other than CAN_MTU 
bytes from the socket is an error. This keeps the code simple, and old 
programs have to be written this way, because there is no way for them 
to guess what not yet implemented CAN versions will look like.

So, this option makes a lot more sense to me now, sorry for the extra 
noise :)


Max