On 14/11/2024 at 18:57, Dan Carpenter wrote:

On Thu, Nov 14, 2024 at 06:34:49PM +0900, Vincent Mailhol wrote:

quoted

Hi Dan,

On 14/11/2024 at 18:03, Dan Carpenter wrote:

quoted

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