linux-kernel - Re: [PATCH net] can: can327: fix snprintf() limit in can327_handle

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <22e388b5-37a1-40a6-bb70-4784e29451ed@enpas.org>
Date: Thu, 14 Nov 2024 18:19:12 +0900
From: Max Staudt <max@...as.org>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Marc Kleine-Budde <mkl@...gutronix.de>,
 Vincent Mailhol <mailhol.vincent@...adoo.fr>,
 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()

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