| Message ID | 20231130105459.3208986-4-lee@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 7d7f794482b74e39d8c0cd830333eb40fc0234d4 |
| Headers | show |
| Series | usb: Replace {v}snprintf() variants with safer alternatives | expand |
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)
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 --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;
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(-)