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
