[0/2] Switch to using scnprintf() in the UHCI driver's debugging code

Here are 2 patches against the 'usb-next' branch of Greg KH's 'usb.git' repo.
The UHCI driver's debugging code uses a lot of sprintf() calls with the large
buffers, leaving some space at the end of the buffers to handle the buffer
overflow.  Using scnprntf() calls instead eliminates the very possibility of
the buffer overflow...

Sergey Shtylyov (2):
  usb: host: uhci-debug: use scnprintf() instead of sprintf()
  usb: host: uhci: remove #define EXTRA_SPACE

 drivers/usb/host/uhci-debug.c | 263 ++++++++++++----------------------
 drivers/usb/host/uhci-hcd.c   |   2 +-
 drivers/usb/host/uhci-q.c     |   2 +-
 3 files changed, 93 insertions(+), 174 deletions(-)

From: Sergey Shtylyov
> Sent: 12 March 2022 20:29
> 
> The UHCI driver's debugging code uses a lot of sprintf() calls with the
> large buffers, leaving some space at the end of the buffers to handle the
> buffer overflow. Using scnprntf() instead eliminates the very possibility
> of the buffer overflow, while simplifying the code at the expense of not
> printing an ellipsis when the end of buffer is actually reached...

Hmmm...

The old code seems to so:

> -	out += sprintf(out, "(buf=%08x)\n", hc32_to_cpu(uhci, td->buffer));
> 
> -	if (out - buf > len)
> -		out += sprintf(out, " ...\n");

Which is going to overflow the output buffer unless there
is enough 'tailroom' after buf[len] for all the sprintf()
before any length check and the ellipsis.

The new code won't overrun buf[len] but also fails to
'\n' terminate long lines.
So you probably do need a check for:
	if (out == len - 1 && buf[out - 1] != '\n')
		strcpy(buf + len - 5, "...\n");

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

On 3/13/22 1:33 AM, David Laight wrote:

[...]
>> The UHCI driver's debugging code uses a lot of sprintf() calls with the
>> large buffers, leaving some space at the end of the buffers to handle the
>> buffer overflow. Using scnprntf() instead eliminates the very possibility
>> of the buffer overflow, while simplifying the code at the expense of not
>> printing an ellipsis when the end of buffer is actually reached...
> 
> Hmmm...
> 
> The old code seems to so:

   s/so/do/? :-)

>> -	out += sprintf(out, "(buf=%08x)\n", hc32_to_cpu(uhci, td->buffer));
>>
>> -	if (out - buf > len)
>> -		out += sprintf(out, " ...\n");
> 
> Which is going to overflow the output buffer unless there
> is enough 'tailroom' after buf[len] for all the sprintf()

   There are 1024 bytes (EXTRA_SPACE)...

> before any length check and the ellipsis.
> 
> The new code won't overrun buf[len] but also fails to
> '\n' terminate long lines.

   Yes.
   And one also has problems correctly identifying the overflowing lines (iff
such line ends exactly at end of buffer)... :-(

> So you probably do need a check for:
> 	if (out == len - 1 && buf[out - 1] != '\n')

   'out' is a pointer, you probably meant:

	if (out - buf == len - 1 && *(out - 1) != '\n')

> 		strcpy(buf + len - 5, "...\n");

   That's not exactly what's done by the existing code... I think we'd be
better off using strrchr()... but then again, we're not sure we have at least
5 bytes...

> 	David

[...]

MBR, Sergey