| Message ID | c896ba5d-7147-4978-9e25-86cfd88ff9dc@stanley.mountain (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] can: can327: fix snprintf() limit in can327_handle_prompt() | expand |
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
On Thu, Nov 14, 2024 at 06:19:12PM +0900, Max Staudt wrote: > 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. So far, so good. But it sounds like you've already written this patch in your head. Can you just send this and give me Reported-by credit? > Also, snprintf() can be > simplified to sprintf(), since it is fully predictable in this case. > I don't love transitions from snprintf() to sprintf() but I won't complain. > > 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? No idea. Can is quite difficult to parse from a static checker point of view because of how it casts skb->data to a struct validates that everything is correct and then passes it around as skb->data. #opaque. Smatch always treats skb->data as untrusted, which is mostly a problem on the send path but with can it's a problem throughout. > > > I can whip something up next week. Excelent, thanks. regards, dan carpenter
To be honest, I was afraid that someone was going to suggest using on of the helper functions that dumps hex. (I don't remember then names of them so that's why I didn't do that). regards, dan carpenter
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've also changed the function from snprintf() to scnprintf(). The > difference between the two functions is that snprintf() returns the number > of bytes which would have been printed if there were space while the > scnprintf() function returns the number of bytes which are actually > printed. > > Fixes: 43da2f07622f ("can: can327: CAN/ldisc driver for ELM327 based OBD-II adapters") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > --- > drivers/net/can/can327.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/can/can327.c b/drivers/net/can/can327.c > index 24af63961030..5c05ebc72318 100644 > --- a/drivers/net/can/can327.c > +++ b/drivers/net/can/can327.c > @@ -623,16 +623,16 @@ static void can327_handle_prompt(struct can327 *elm) > snprintf(local_txbuf, sizeof(local_txbuf), "ATRTR\r"); > } else { > /* Send a regular CAN data frame */ > + int off = 0; > int i; > > for (i = 0; i < frame->len; i++) { > - snprintf(&local_txbuf[2 * i], > - sizeof(local_txbuf), "%02X", > - frame->data[i]); > + off += scnprintf(&local_txbuf[off], > + sizeof(local_txbuf) - off, > + "%02X", frame->data[i]); > } > > - snprintf(&local_txbuf[2 * i], sizeof(local_txbuf), > - "\r"); > + scnprintf(&local_txbuf[off], sizeof(local_txbuf) - off, "\r"); > } > > elm->drop_next_line = 1; Yours sincerely, Vincent Mailhol
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. > 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. regards, dan carpenter
On 11/14/24 18:29, Dan Carpenter wrote: > To be honest, I was afraid that someone was going to suggest using on of the > helper functions that dumps hex. (I don't remember then names of them so that's > why I didn't do that). If you can think of a neat, clearer replacement for sprintf() in this case, that'd be nice. I guess I didn't find one at the time when I wrote the driver (or I didn't look hard enough). Suggestions welcome! Max
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. Yours sincerely, Vincent Mailhol
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? regards, Marc
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
I'm happy to re-write the commit message. Changing snprintf to sprintf() makes me so much less happy... regards, dan carpenter
On 15/11/2024 at 00:08, Dan Carpenter wrote: > I'm happy to re-write the commit message. Changing snprintf to sprintf() makes > me so much less happy... OK. Let me amend my previous message. I kind of understood from the past exchanges that Max will take the ownership of this patch and credit you a with a Reported-by: tag. If you keep the ownership of the patch, then that's a different story :) I do not want to make you sad and I am fine with your preferred approach. Yours sincerely, Vincent Mailhol
On Fri, Nov 15, 2024 at 12:24:17AM +0900, Vincent Mailhol wrote: > On 15/11/2024 at 00:08, Dan Carpenter wrote: > > I'm happy to re-write the commit message. Changing snprintf to sprintf() makes > > me so much less happy... > > OK. Let me amend my previous message. I kind of understood from the past > exchanges that Max will take the ownership of this patch and credit you > a with a Reported-by: tag. > > If you keep the ownership of the patch, then that's a different story :) > > I do not want to make you sad and I am fine with your preferred approach. Then I can just resend this tomorrow. regards, dan carpenter
Hi all, As promised, here is a patch cleaning up can327's payload "encoding" (the hex dump part), plus a comment explaining why Dan's finding turned out not to be security relevant. It's as Vincent already explained, plus additional background information: https://lore.kernel.org/linux-can/20241119003815.767004-1-max@enpas.org/T/ I've taken the liberty of not CC'ing the network maintainers on that patch, hence this email with a pointer to it for anyone interested. In the end, while it looked worrying at first, it ended up being just a minor cleanup. Thanks Dan for pointing out that ugly piece of code. I'd really like to one day find the time to do some further cleanup, and especially further commenting in order to reduce the bus factor, but oh well... Max
diff --git a/drivers/net/can/can327.c b/drivers/net/can/can327.c index 24af63961030..5c05ebc72318 100644 --- a/drivers/net/can/can327.c +++ b/drivers/net/can/can327.c @@ -623,16 +623,16 @@ static void can327_handle_prompt(struct can327 *elm) snprintf(local_txbuf, sizeof(local_txbuf), "ATRTR\r"); } else { /* Send a regular CAN data frame */ + int off = 0; int i; for (i = 0; i < frame->len; i++) { - snprintf(&local_txbuf[2 * i], - sizeof(local_txbuf), "%02X", - frame->data[i]); + off += scnprintf(&local_txbuf[off], + sizeof(local_txbuf) - off, + "%02X", frame->data[i]); } - snprintf(&local_txbuf[2 * i], sizeof(local_txbuf), - "\r"); + scnprintf(&local_txbuf[off], sizeof(local_txbuf) - off, "\r"); } elm->drop_next_line = 1;
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. I've also changed the function from snprintf() to scnprintf(). The difference between the two functions is that snprintf() returns the number of bytes which would have been printed if there were space while the scnprintf() function returns the number of bytes which are actually printed. Fixes: 43da2f07622f ("can: can327: CAN/ldisc driver for ELM327 based OBD-II adapters") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- --- drivers/net/can/can327.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)