Re: [PATCH net] can: can327: fix snprintf() limit in can327_handle_prompt()

[Vincent Mailhol <mailhol.vincent@wanadoo.fr>]

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