Message-ID: <7841268c-c8dc-4db9-b2dd-c2c5fc366022@wanadoo.fr>
Date: Thu, 14 Nov 2024 23:54:11 +0900
From: Vincent Mailhol <mailhol.vincent@...adoo.fr>
To: Marc Kleine-Budde <mkl@...gutronix.de>
Cc: Dan Carpenter <dan.carpenter@...aro.org>, Max Staudt <max@...as.org>,
 Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 linux-can@...r.kernel.org, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH net] can: can327: fix snprintf() limit in
 can327_handle_prompt()

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

Powered by blists - more mailing lists