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@xxxxxxxxxx>
---
---
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
