diff mbox series

[3/5] usb: fotg210-hcd: Replace snprintf() with the safer scnprintf() variant

Message ID 20231130105459.3208986-4-lee@kernel.org
State New
Headers
Series usb: Replace {v}snprintf() variants with safer alternatives |

Commit Message

Lee Jones Nov. 30, 2023, 10:54 a.m. UTC 
  There is a general misunderstanding amongst engineers that {v}snprintf()
returns the length of the data *actually* encoded into the destination
array.  However, as per the C99 standard {v}snprintf() really returns
the length of the data that *would have been* written if there were
enough space for it.  This misunderstanding has led to buffer-overruns
in the past.  It's generally considered safer to use the {v}scnprintf()
variants in their place (or even sprintf() in simple cases).  So let's
do that.

The uses in this file both seem to assume that data *has been* written!

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Yuan-Hsin Chen <yhchen@faraday-tech.com>
Cc: Feng-Hsin Chiang <john453@faraday-tech.com>
Cc: Po-Yu Chuang <ratbert.chuang@gmail.com>
Cc: linux-usb@vger.kernel.org
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/usb/fotg210/fotg210-hcd.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Comments

David Laight Nov. 30, 2023, 2:40 p.m. UTC | #1 
From: Lee Jones
> Sent: 30 November 2023 10:55
> 
> There is a general misunderstanding amongst engineers that {v}snprintf()
> returns the length of the data *actually* encoded into the destination
> array.  However, as per the C99 standard {v}snprintf() really returns
> the length of the data that *would have been* written if there were
> enough space for it.  This misunderstanding has led to buffer-overruns
> in the past.  It's generally considered safer to use the {v}scnprintf()
> variants in their place (or even sprintf() in simple cases).  So let's
> do that.
> 
> The uses in this file both seem to assume that data *has been* written!
...
> -		temp = snprintf(next, size,
> -				"\n\t%p%c%s len=%d %08x urb %p",
> -				td, mark, ({ char *tmp;
...
> -		if (size < temp)
> -			temp = size;

That is actually a bug - even though it is trying to be correct.
The trailing '\0' that snprintf() adds (unless you are using the
M$ one) will end up in the buffer.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Linus Walleij Nov. 30, 2023, 2:48 p.m. UTC | #2 
On Thu, Nov 30, 2023 at 11:55 AM Lee Jones <lee@kernel.org> wrote:

> There is a general misunderstanding amongst engineers that {v}snprintf()
> returns the length of the data *actually* encoded into the destination
> array.  However, as per the C99 standard {v}snprintf() really returns
> the length of the data that *would have been* written if there were
> enough space for it.  This misunderstanding has led to buffer-overruns
> in the past.  It's generally considered safer to use the {v}scnprintf()
> variants in their place (or even sprintf() in simple cases).  So let's
> do that.
>
> The uses in this file both seem to assume that data *has been* written!
>
> Link: https://lwn.net/Articles/69419/
> Link: https://github.com/KSPP/linux/issues/105
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Yuan-Hsin Chen <yhchen@faraday-tech.com>
> Cc: Feng-Hsin Chiang <john453@faraday-tech.com>
> Cc: Po-Yu Chuang <ratbert.chuang@gmail.com>
> Cc: linux-usb@vger.kernel.org
> Signed-off-by: Lee Jones <lee@kernel.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks for doing this Lee!

And as David points out it is even a bug fix at the same time.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/usb/fotg210/fotg210-hcd.c b/drivers/usb/fotg210/fotg210-hcd.c
index 929106c16b29b..b2f8b53cc8ef5 100644
--- a/drivers/usb/fotg210/fotg210-hcd.c
+++ b/drivers/usb/fotg210/fotg210-hcd.c
@@ -404,9 +404,9 @@  static void qh_lines(struct fotg210_hcd *fotg210, struct fotg210_qh *qh,
 			else if (td->hw_alt_next != list_end)
 				mark = '/';
 		}
-		temp = snprintf(next, size,
-				"\n\t%p%c%s len=%d %08x urb %p",
-				td, mark, ({ char *tmp;
+		temp = scnprintf(next, size,
+				 "\n\t%p%c%s len=%d %08x urb %p",
+				 td, mark, ({ char *tmp;
 				switch ((scratch>>8)&0x03) {
 				case 0:
 					tmp = "out";
@@ -424,17 +424,13 @@  static void qh_lines(struct fotg210_hcd *fotg210, struct fotg210_qh *qh,
 				(scratch >> 16) & 0x7fff,
 				scratch,
 				td->urb);
-		if (size < temp)
-			temp = size;
 		size -= temp;
 		next += temp;
 		if (temp == size)
 			goto done;
 	}
 
-	temp = snprintf(next, size, "\n");
-	if (size < temp)
-		temp = size;
+	temp = scnprintf(next, size, "\n");
 
 	size -= temp;
 	next += temp;