This code is printing hex values to the &local_txbuf buffer and it's
using the snprintf() function to try prevent buffer overflows.  The
problem is that it's not passing the correct limit to the snprintf()
function so the limit doesn't do anything.  On each iteration we print
two digits so the remaining size should also decrease by two, but
instead it passes the sizeof() the entire buffer each time.

If the frame->len were too long it would result in a buffer overflow.

I've also changed the function from snprintf() to scnprintf().  The
difference between the two functions is that snprintf() returns the number
of bytes which would have been printed if there were space while the
scnprintf() function returns the number of bytes which are actually
printed.

Fixes: 43da2f07622f ("can: can327: CAN/ldisc driver for ELM327 based OBD-II adapters")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
---
 drivers/net/can/can327.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/can327.c b/drivers/net/can/can327.c
index 24af63961030..5c05ebc72318 100644
--- a/drivers/net/can/can327.c
+++ b/drivers/net/can/can327.c
@@ -623,16 +623,16 @@ static void can327_handle_prompt(struct can327 *elm)
 			snprintf(local_txbuf, sizeof(local_txbuf), "ATRTR\r");
 		} else {
 			/* Send a regular CAN data frame */
+			int off = 0;
 			int i;
 
 			for (i = 0; i < frame->len; i++) {
-				snprintf(&local_txbuf[2 * i],
-					 sizeof(local_txbuf), "%02X",
-					 frame->data[i]);
+				off += scnprintf(&local_txbuf[off],
+						 sizeof(local_txbuf) - off,
+						 "%02X", frame->data[i]);
 			}
 
-			snprintf(&local_txbuf[2 * i], sizeof(local_txbuf),
-				 "\r");
+			scnprintf(&local_txbuf[off], sizeof(local_txbuf) - off, "\r");
 		}
 
 		elm->drop_next_line = 1;
-- 
2.45.2

Hi Dan,

On 14/11/2024 at 18:03, Dan Carpenter wrote:
> This code is printing hex values to the &local_txbuf buffer and it's
> using the snprintf() function to try prevent buffer overflows.  The
> problem is that it's not passing the correct limit to the snprintf()
> function so the limit doesn't do anything.  On each iteration we print
> two digits so the remaining size should also decrease by two, but
> instead it passes the sizeof() the entire buffer each time.
> 
> If the frame->len were too long it would result in a buffer overflow.

But, can frame->len be too long? Classical CAN frame maximum length is 8 
bytes. And I do not see a path for a malformed frame to reach this part 
of the driver.

If such a path exists, I think this should be explained. Else, I am just 
not sure if this needs a Fixes: tag.

> I've also changed the function from snprintf() to scnprintf().  The
> difference between the two functions is that snprintf() returns the number
> of bytes which would have been printed if there were space while the
> scnprintf() function returns the number of bytes which are actually
> printed.
> 
> Fixes: 43da2f07622f ("can: can327: CAN/ldisc driver for ELM327 based OBD-II adapters")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> ---
>   drivers/net/can/can327.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/can/can327.c b/drivers/net/can/can327.c
> index 24af63961030..5c05ebc72318 100644
> --- a/drivers/net/can/can327.c
> +++ b/drivers/net/can/can327.c
> @@ -623,16 +623,16 @@ static void can327_handle_prompt(struct can327 *elm)
>   			snprintf(local_txbuf, sizeof(local_txbuf), "ATRTR\r");
>   		} else {
>   			/* Send a regular CAN data frame */
> +			int off = 0;
>   			int i;
>   
>   			for (i = 0; i < frame->len; i++) {
> -				snprintf(&local_txbuf[2 * i],
> -					 sizeof(local_txbuf), "%02X",
> -					 frame->data[i]);
> +				off += scnprintf(&local_txbuf[off],
> +						 sizeof(local_txbuf) - off,
> +						 "%02X", frame->data[i]);
>   			}
>   
> -			snprintf(&local_txbuf[2 * i], sizeof(local_txbuf),
> -				 "\r");
> +			scnprintf(&local_txbuf[off], sizeof(local_txbuf) - off, "\r");
>   		}
>   
>   		elm->drop_next_line = 1;

Yours sincerely,
Vincent Mailhol

On Thu, Nov 14, 2024 at 06:34:49PM +0900, Vincent Mailhol wrote:
> Hi Dan,
> 
> On 14/11/2024 at 18:03, Dan Carpenter wrote:
> > This code is printing hex values to the &local_txbuf buffer and it's
> > using the snprintf() function to try prevent buffer overflows.  The
> > problem is that it's not passing the correct limit to the snprintf()
> > function so the limit doesn't do anything.  On each iteration we print
> > two digits so the remaining size should also decrease by two, but
> > instead it passes the sizeof() the entire buffer each time.
> > 
> > If the frame->len were too long it would result in a buffer overflow.
> 
> But, can frame->len be too long? Classical CAN frame maximum length is 8
> bytes. And I do not see a path for a malformed frame to reach this part of
> the driver.
> 
> If such a path exists, I think this should be explained. Else, I am just not
> sure if this needs a Fixes: tag.
> 

Even when bugs don't affect runtime we still assign a Fixes tag, but we don't
CC stable.  There is no way that passing the wrong size was intentional.

regards,
dan carpenter

On 14/11/2024 at 18:57, Dan Carpenter wrote:
> On Thu, Nov 14, 2024 at 06:34:49PM +0900, Vincent Mailhol wrote:
>> Hi Dan,
>>
>> On 14/11/2024 at 18:03, Dan Carpenter wrote:
>>> This code is printing hex values to the &local_txbuf buffer and it's
>>> using the snprintf() function to try prevent buffer overflows.  The
>>> problem is that it's not passing the correct limit to the snprintf()
>>> function so the limit doesn't do anything.  On each iteration we print
>>> two digits so the remaining size should also decrease by two, but
>>> instead it passes the sizeof() the entire buffer each time.
>>>
>>> If the frame->len were too long it would result in a buffer overflow.
>>
>> But, can frame->len be too long? Classical CAN frame maximum length is 8
>> bytes. And I do not see a path for a malformed frame to reach this part of
>> the driver.
>>
>> If such a path exists, I think this should be explained. Else, I am just not
>> sure if this needs a Fixes: tag.

I confirmed the CAN frame length is correctly checked.

The only way to trigger that snprintf() with the wrong size is if 
CAN327_TX_DO_CAN_DATA is set, which only occurs in can327_send_frame(). 
And the only caller of can327_send_frame() is can327_netdev_start_xmit().

can327_netdev_start_xmit() calls can_dev_dropped_skb() which in turn 
calls can_dropped_invalid_skb() which goes to can_is_can_skb() which 
finally checks that cf->len is not bigger than CAN_MAX_DLEN (i.e. 8 bytes).

So indeed, no buffer overflow can occur here.

> Even when bugs don't affect runtime we still assign a Fixes tag, but we don't
> CC stable.  There is no way that passing the wrong size was intentional.

Got it. Thanks for the explanation, now it makes sense to keep the 
Fixes: tag.


Yours sincerely,
Vincent Mailhol

On 14.11.2024 21:35:07, Vincent Mailhol wrote:
> On 14/11/2024 at 18:57, Dan Carpenter wrote:
> > On Thu, Nov 14, 2024 at 06:34:49PM +0900, Vincent Mailhol wrote:
> > > Hi Dan,
> > > 
> > > On 14/11/2024 at 18:03, Dan Carpenter wrote:
> > > > This code is printing hex values to the &local_txbuf buffer and it's
> > > > using the snprintf() function to try prevent buffer overflows.  The
> > > > problem is that it's not passing the correct limit to the snprintf()
> > > > function so the limit doesn't do anything.  On each iteration we print
> > > > two digits so the remaining size should also decrease by two, but
> > > > instead it passes the sizeof() the entire buffer each time.
> > > > 
> > > > If the frame->len were too long it would result in a buffer overflow.
> > > 
> > > But, can frame->len be too long? Classical CAN frame maximum length is 8
> > > bytes. And I do not see a path for a malformed frame to reach this part of
> > > the driver.
> > > 
> > > If such a path exists, I think this should be explained. Else, I am just not
> > > sure if this needs a Fixes: tag.
> 
> I confirmed the CAN frame length is correctly checked.
> 
> The only way to trigger that snprintf() with the wrong size is if
> CAN327_TX_DO_CAN_DATA is set, which only occurs in can327_send_frame(). And
> the only caller of can327_send_frame() is can327_netdev_start_xmit().
> 
> can327_netdev_start_xmit() calls can_dev_dropped_skb() which in turn calls
> can_dropped_invalid_skb() which goes to can_is_can_skb() which finally
> checks that cf->len is not bigger than CAN_MAX_DLEN (i.e. 8 bytes).
> 
> So indeed, no buffer overflow can occur here.
> 
> > Even when bugs don't affect runtime we still assign a Fixes tag, but we don't
> > CC stable.  There is no way that passing the wrong size was intentional.
> 
> Got it. Thanks for the explanation, now it makes sense to keep the Fixes:
> tag.

Should we take the patch as it is?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

On 14/11/2024 at 22:34, Marc Kleine-Budde wrote:
> On 14.11.2024 21:35:07, Vincent Mailhol wrote:
>> On 14/11/2024 at 18:57, Dan Carpenter wrote:
>>> On Thu, Nov 14, 2024 at 06:34:49PM +0900, Vincent Mailhol wrote:
>>>> Hi Dan,
>>>>
>>>> On 14/11/2024 at 18:03, Dan Carpenter wrote:
>>>>> This code is printing hex values to the &local_txbuf buffer and it's
>>>>> using the snprintf() function to try prevent buffer overflows.  The
>>>>> problem is that it's not passing the correct limit to the snprintf()
>>>>> function so the limit doesn't do anything.  On each iteration we print
>>>>> two digits so the remaining size should also decrease by two, but
>>>>> instead it passes the sizeof() the entire buffer each time.
>>>>>
>>>>> If the frame->len were too long it would result in a buffer overflow.
>>>>
>>>> But, can frame->len be too long? Classical CAN frame maximum length is 8
>>>> bytes. And I do not see a path for a malformed frame to reach this part of
>>>> the driver.
>>>>
>>>> If such a path exists, I think this should be explained. Else, I am just not
>>>> sure if this needs a Fixes: tag.
>>
>> I confirmed the CAN frame length is correctly checked.
>>
>> The only way to trigger that snprintf() with the wrong size is if
>> CAN327_TX_DO_CAN_DATA is set, which only occurs in can327_send_frame(). And
>> the only caller of can327_send_frame() is can327_netdev_start_xmit().
>>
>> can327_netdev_start_xmit() calls can_dev_dropped_skb() which in turn calls
>> can_dropped_invalid_skb() which goes to can_is_can_skb() which finally
>> checks that cf->len is not bigger than CAN_MAX_DLEN (i.e. 8 bytes).
>>
>> So indeed, no buffer overflow can occur here.
>>
>>> Even when bugs don't affect runtime we still assign a Fixes tag, but we don't
>>> CC stable.  There is no way that passing the wrong size was intentional.
>>
>> Got it. Thanks for the explanation, now it makes sense to keep the Fixes:
>> tag.
> 
> Should we take the patch as it is?

I am not keen of taking it as-is. *At least*, I think that the 
description should be updated to say that this bug can *not* result in a 
buffer overflow because the frame length limit of eight bytes is 
enforced by can_dev_dropped_skb(). If we keep things as-is, I am worried 
that we will create additional work for the CVE team.

As for the code itself, why not, but I prefer the suggestion made by 
Max. If the length can not exceed eight bytes, why writing code to 
handle an otherwise impossible to trigger condition?

I also quickly looked at the hexdump helper functions and found bin2hex():

   https://elixir.bootlin.com/linux/v6.11/source/lib/hexdump.c#L87

It is promissing on first sight, but it produces lower case hexadecimal. 
And it doesn't look like the can327 would accept that.

At the end, I am fine to defer to Max the final decision on what to do 
on the code. At the end, he is the maintainer of that module.


Yours sincerely,
Vincent Mailhol

I'm happy to re-write the commit message.  Changing snprintf to sprintf() makes
me so much less happy...

regards,
dan carpenter

On 15/11/2024 at 00:08, Dan Carpenter wrote:
> I'm happy to re-write the commit message.  Changing snprintf to sprintf() makes
> me so much less happy...

OK. Let me amend my previous message. I kind of understood from the past
exchanges that Max will take the ownership of this patch and credit you
a with a Reported-by: tag.

If you keep the ownership of the patch, then that's a different story :)

I do not want to make you sad and I am fine with your preferred approach.


Yours sincerely,
Vincent Mailhol

On Fri, Nov 15, 2024 at 12:24:17AM +0900, Vincent Mailhol wrote:
> On 15/11/2024 at 00:08, Dan Carpenter wrote:
> > I'm happy to re-write the commit message.  Changing snprintf to sprintf() makes
> > me so much less happy...
> 
> OK. Let me amend my previous message. I kind of understood from the past
> exchanges that Max will take the ownership of this patch and credit you
> a with a Reported-by: tag.
> 
> If you keep the ownership of the patch, then that's a different story :)
> 
> I do not want to make you sad and I am fine with your preferred approach.

Then I can just resend this tomorrow.

regards,
dan carpenter

Hi all,

As promised, here is a patch cleaning up can327's payload "encoding" 
(the hex dump part), plus a comment explaining why Dan's finding turned 
out not to be security relevant. It's as Vincent already explained, plus 
additional background information:

  
https://lore.kernel.org/linux-can/20241119003815.767004-1-max@enpas.org/T/

I've taken the liberty of not CC'ing the network maintainers on that 
patch, hence this email with a pointer to it for anyone interested. In 
the end, while it looked worrying at first, it ended up being just a 
minor cleanup.


Thanks Dan for pointing out that ugly piece of code. I'd really like to 
one day find the time to do some further cleanup, and especially further 
commenting in order to reduce the bus factor, but oh well...


Max

Hi Dan,

On 11/14/24 18:03, Dan Carpenter wrote:
> This code is printing hex values to the &local_txbuf buffer and it's
> using the snprintf() function to try prevent buffer overflows.  The
> problem is that it's not passing the correct limit to the snprintf()
> function so the limit doesn't do anything.  On each iteration we print
> two digits so the remaining size should also decrease by two, but
> instead it passes the sizeof() the entire buffer each time.

D'oh, silly mistake. Thank you for finding it!


IMHO the correct fix isn't further counting and checking within the 
snprintf loop. Instead, the buffer is correctly sized for a payload of 
up to 8 bytes, and what we should do is to initially establish that 
frame->len is indeed no larger than 8 bytes. So, something like

if (frame->len > 8) {
	netdev_err(elm->dev, "The CAN stack handed us a frame with len > 8 
bytes. Dropped packet.\n");
}

This check would go into can327_netdev_start_xmit(), and then a comment 
at your current patch's location to remind of this. Also, snprintf() can 
be simplified to sprintf(), since it is fully predictable in this case.


It's also possible that the CAN stack already checks frame->len, in 
which case I'd just add comments to can327. I haven't dug into the code 
now - maybe the maintainers know?


I can whip something up next week.


Max

On Thu, Nov 14, 2024 at 06:19:12PM +0900, Max Staudt wrote:
> Hi Dan,
> 
> On 11/14/24 18:03, Dan Carpenter wrote:
> > This code is printing hex values to the &local_txbuf buffer and it's
> > using the snprintf() function to try prevent buffer overflows.  The
> > problem is that it's not passing the correct limit to the snprintf()
> > function so the limit doesn't do anything.  On each iteration we print
> > two digits so the remaining size should also decrease by two, but
> > instead it passes the sizeof() the entire buffer each time.
> 
> D'oh, silly mistake. Thank you for finding it!
> 
> 
> IMHO the correct fix isn't further counting and checking within the snprintf
> loop. Instead, the buffer is correctly sized for a payload of up to 8 bytes,
> and what we should do is to initially establish that frame->len is indeed no
> larger than 8 bytes. So, something like
> 
> if (frame->len > 8) {
> 	netdev_err(elm->dev, "The CAN stack handed us a frame with len > 8 bytes.
> Dropped packet.\n");
> }
> 
> This check would go into can327_netdev_start_xmit(), and then a comment at
> your current patch's location to remind of this.

So far, so good.  But it sounds like you've already written this patch in your
head.  Can you just send this and give me Reported-by credit?

> Also, snprintf() can be
> simplified to sprintf(), since it is fully predictable in this case.
> 

I don't love transitions from snprintf() to sprintf() but I won't complain.

> 
> It's also possible that the CAN stack already checks frame->len, in which
> case I'd just add comments to can327. I haven't dug into the code now -
> maybe the maintainers know?

No idea.  Can is quite difficult to parse from a static checker point of view
because of how it casts skb->data to a struct validates that everything is
correct and then passes it around as skb->data.  #opaque.  Smatch always treats
skb->data as untrusted, which is mostly a problem on the send path but with can
it's a problem throughout.

> 
> 
> I can whip something up next week.

Excelent, thanks.

regards,
dan carpenter

To be honest, I was afraid that someone was going to suggest using on of the
helper functions that dumps hex.  (I don't remember then names of them so that's
why I didn't do that).

regards,
dan carpenter

On 11/14/24 18:29, Dan Carpenter wrote:
> To be honest, I was afraid that someone was going to suggest using on of the
> helper functions that dumps hex.  (I don't remember then names of them so that's
> why I didn't do that).

If you can think of a neat, clearer replacement for sprintf() in this 
case, that'd be nice. I guess I didn't find one at the time when I wrote 
the driver (or I didn't look hard enough).

Suggestions welcome!


Max