diff mbox series

usb: core: devices: use scnprintf() instead of sprintf()

Message ID ebfd7a94-34d2-6259-fa0c-4a5dcc649d2b@omp.ru (mailing list archive)
State New, archived
Headers show
Series usb: core: devices: use scnprintf() instead of sprintf() | expand

Commit Message

Sergey Shtylyov April 19, 2022, 7:50 p.m. UTC 
The USB device dump code uses the sprintf() calls with a 2-page buffer,
leaving 256 bytes at the end of that buffer to prevent buffer overflow.
Using scnprntf() instead eliminates the very possibility of the buffer
overflow, while also simplifying the code. This however is achieved at
the expense of not printing the "(truncated)" line anymore when the end
of that buffer is actually reached; instead a possible partial line at
the end of buffer (not ending with '\n') is now not printed.

Found by Linux Verification Center (linuxtesting.org) with the SVACE static
analysis tool.

Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>

---
This patch is against the 'usb-next' branch of Greg KH's 'usb.git' repo,
plus the patch removing some dead code I posted yesterday -- there's no
context dependency, only a single hunk's offset though...

 drivers/usb/core/devices.c |  138 +++++++++++++++++----------------------------
 1 file changed, 54 insertions(+), 84 deletions(-)

Comments

Greg KH April 20, 2022, 6:40 a.m. UTC | #1 
On Tue, Apr 19, 2022 at 10:50:22PM +0300, Sergey Shtylyov wrote:
> The USB device dump code uses the sprintf() calls with a 2-page buffer,
> leaving 256 bytes at the end of that buffer to prevent buffer overflow.
> Using scnprntf() instead eliminates the very possibility of the buffer
> overflow, while also simplifying the code. This however is achieved at
> the expense of not printing the "(truncated)" line anymore when the end
> of that buffer is actually reached; instead a possible partial line at
> the end of buffer (not ending with '\n') is now not printed.

So you just changed a user-visable abi :(

Please no, obviously that is not allowed.

greg k-h
Sergey Shtylyov April 20, 2022, 8:04 p.m. UTC | #2 
Hello!

On 4/20/22 9:40 AM, Greg Kroah-Hartman wrote:

   Thanks for the (unusually?) prompt reply! :-)

>> The USB device dump code uses the sprintf() calls with a 2-page buffer,
>> leaving 256 bytes at the end of that buffer to prevent buffer overflow.
>> Using scnprntf() instead eliminates the very possibility of the buffer
>> overflow, while also simplifying the code. This however is achieved at
>> the expense of not printing the "(truncated)" line anymore when the end
>> of that buffer is actually reached; instead a possible partial line at
>> the end of buffer (not ending with '\n') is now not printed.
> 
> So you just changed a user-visable abi :(

    debugfs is an ABI too? :-)
 
> Please no, obviously that is not allowed.

    Oh, well...
   I'll prepare another patch then: some of the checks in this file are
clearly redundant...

> greg k-h

MBR, Sergey
Greg KH April 21, 2022, 6:10 a.m. UTC | #3 
On Wed, Apr 20, 2022 at 11:04:00PM +0300, Sergey Shtylyov wrote:
> Hello!
> 
> On 4/20/22 9:40 AM, Greg Kroah-Hartman wrote:
> 
>    Thanks for the (unusually?) prompt reply! :-)
> 
> >> The USB device dump code uses the sprintf() calls with a 2-page buffer,
> >> leaving 256 bytes at the end of that buffer to prevent buffer overflow.
> >> Using scnprntf() instead eliminates the very possibility of the buffer
> >> overflow, while also simplifying the code. This however is achieved at
> >> the expense of not printing the "(truncated)" line anymore when the end
> >> of that buffer is actually reached; instead a possible partial line at
> >> the end of buffer (not ending with '\n') is now not printed.
> > 
> > So you just changed a user-visable abi :(
> 
>     debugfs is an ABI too? :-)

When we have tools that we know parse it, yes it can be.

But here you are just changing the existing format for no good reason,
which is generally considered a bad thing.

thanks,

greg k-h
diff mbox series

Patch

Index: usb/drivers/usb/core/devices.c
===================================================================
--- usb.orig/drivers/usb/core/devices.c
+++ usb/drivers/usb/core/devices.c
@@ -145,9 +145,6 @@  static char *usb_dump_endpoint_descripto
 	char dir, unit, *type;
 	unsigned interval, bandwidth = 1;
 
-	if (start > end)
-		return start;
-
 	dir = usb_endpoint_dir_in(desc) ? 'I' : 'O';
 
 	if (speed == USB_SPEED_HIGH)
@@ -180,11 +177,11 @@  static char *usb_dump_endpoint_descripto
 		interval /= 1000;
 	}
 
-	start += sprintf(start, format_endpt, desc->bEndpointAddress, dir,
-			 desc->bmAttributes, type,
-			 usb_endpoint_maxp(desc) *
-			 bandwidth,
-			 interval, unit);
+	start += scnprintf(start, end - start, format_endpt,
+			   desc->bEndpointAddress, dir,
+			   desc->bmAttributes, type,
+			   usb_endpoint_maxp(desc) * bandwidth,
+			   interval, unit);
 	return start;
 }
 
@@ -197,8 +194,6 @@  static char *usb_dump_interface_descript
 	const char *driver_name = "";
 	int active = 0;
 
-	if (start > end)
-		return start;
 	desc = &intfc->altsetting[setno].desc;
 	if (iface) {
 		driver_name = (iface->dev.driver
@@ -206,16 +201,16 @@  static char *usb_dump_interface_descript
 				: "(none)");
 		active = (desc == &iface->cur_altsetting->desc);
 	}
-	start += sprintf(start, format_iface,
-			 active ? '*' : ' ',	/* mark active altsetting */
-			 desc->bInterfaceNumber,
-			 desc->bAlternateSetting,
-			 desc->bNumEndpoints,
-			 desc->bInterfaceClass,
-			 class_decode(desc->bInterfaceClass),
-			 desc->bInterfaceSubClass,
-			 desc->bInterfaceProtocol,
-			 driver_name);
+	start += scnprintf(start, end - start, format_iface,
+			   active ? '*' : ' ',	/* mark active altsetting */
+			   desc->bInterfaceNumber,
+			   desc->bAlternateSetting,
+			   desc->bNumEndpoints,
+			   desc->bInterfaceClass,
+			   class_decode(desc->bInterfaceClass),
+			   desc->bInterfaceSubClass,
+			   desc->bInterfaceProtocol,
+			   driver_name);
 	return start;
 }
 
@@ -228,8 +223,6 @@  static char *usb_dump_interface(int spee
 
 	start = usb_dump_interface_descriptor(start, end, intfc, iface, setno);
 	for (i = 0; i < desc->desc.bNumEndpoints; i++) {
-		if (start > end)
-			return start;
 		start = usb_dump_endpoint_descriptor(speed,
 				start, end, &desc->endpoint[i].desc);
 	}
@@ -239,15 +232,13 @@  static char *usb_dump_interface(int spee
 static char *usb_dump_iad_descriptor(char *start, char *end,
 			const struct usb_interface_assoc_descriptor *iad)
 {
-	if (start > end)
-		return start;
-	start += sprintf(start, format_iad,
-			 iad->bFirstInterface,
-			 iad->bInterfaceCount,
-			 iad->bFunctionClass,
-			 class_decode(iad->bFunctionClass),
-			 iad->bFunctionSubClass,
-			 iad->bFunctionProtocol);
+	start += scnprintf(start, end - start, format_iad,
+			   iad->bFirstInterface,
+			   iad->bInterfaceCount,
+			   iad->bFunctionClass,
+			   class_decode(iad->bFunctionClass),
+			   iad->bFunctionSubClass,
+			   iad->bFunctionProtocol);
 	return start;
 }
 
@@ -262,19 +253,17 @@  static char *usb_dump_config_descriptor(
 {
 	int mul;
 
-	if (start > end)
-		return start;
 	if (speed >= USB_SPEED_SUPER)
 		mul = 8;
 	else
 		mul = 2;
-	start += sprintf(start, format_config,
-			 /* mark active/actual/current cfg. */
-			 active ? '*' : ' ',
-			 desc->bNumInterfaces,
-			 desc->bConfigurationValue,
-			 desc->bmAttributes,
-			 desc->bMaxPower * mul);
+	start += scnprintf(start, end - start, format_config,
+			   /* mark active/actual/current cfg. */
+			   active ? '*' : ' ',
+			   desc->bNumInterfaces,
+			   desc->bConfigurationValue,
+			   desc->bmAttributes,
+			   desc->bMaxPower * mul);
 	return start;
 }
 
@@ -285,11 +274,9 @@  static char *usb_dump_config(int speed,
 	struct usb_interface_cache *intfc;
 	struct usb_interface *interface;
 
-	if (start > end)
-		return start;
 	if (!config)
 		/* getting these some in 2.3.7; none in 2.3.6 */
-		return start + sprintf(start, "(null Cfg. desc.)\n");
+		return start + scnprintf(start, end - start, "(null Cfg. desc.)\n");
 	start = usb_dump_config_descriptor(start, end, &config->desc, active,
 			speed);
 	for (i = 0; i < USB_MAXIADS; i++) {
@@ -302,8 +289,6 @@  static char *usb_dump_config(int speed,
 		intfc = config->intf_cache[i];
 		interface = config->interface[i];
 		for (j = 0; j < intfc->num_altsetting; j++) {
-			if (start > end)
-				return start;
 			start = usb_dump_interface(speed,
 				start, end, intfc, interface, j);
 		}
@@ -320,22 +305,18 @@  static char *usb_dump_device_descriptor(
 	u16 bcdUSB = le16_to_cpu(desc->bcdUSB);
 	u16 bcdDevice = le16_to_cpu(desc->bcdDevice);
 
-	if (start > end)
-		return start;
-	start += sprintf(start, format_device1,
-			  bcdUSB >> 8, bcdUSB & 0xff,
-			  desc->bDeviceClass,
-			  class_decode(desc->bDeviceClass),
-			  desc->bDeviceSubClass,
-			  desc->bDeviceProtocol,
-			  desc->bMaxPacketSize0,
-			  desc->bNumConfigurations);
-	if (start > end)
-		return start;
-	start += sprintf(start, format_device2,
-			 le16_to_cpu(desc->idVendor),
-			 le16_to_cpu(desc->idProduct),
-			 bcdDevice >> 8, bcdDevice & 0xff);
+	start += scnprintf(start, end - start, format_device1,
+			   bcdUSB >> 8, bcdUSB & 0xff,
+			   desc->bDeviceClass,
+			   class_decode(desc->bDeviceClass),
+			   desc->bDeviceSubClass,
+			   desc->bDeviceProtocol,
+			   desc->bMaxPacketSize0,
+			   desc->bNumConfigurations);
+	start += scnprintf(start, end - start, format_device2,
+			   le16_to_cpu(desc->idVendor),
+			   le16_to_cpu(desc->idProduct),
+			   bcdDevice >> 8, bcdDevice & 0xff);
 	return start;
 }
 
@@ -345,23 +326,17 @@  static char *usb_dump_device_descriptor(
 static char *usb_dump_device_strings(char *start, char *end,
 				     struct usb_device *dev)
 {
-	if (start > end)
-		return start;
 	if (dev->manufacturer)
-		start += sprintf(start, format_string_manufacturer,
+		start += scnprintf(start, end - start, format_string_manufacturer,
 				 dev->manufacturer);
-	if (start > end)
-		goto out;
 	if (dev->product)
-		start += sprintf(start, format_string_product, dev->product);
-	if (start > end)
-		goto out;
+		start += scnprintf(start, end - start, format_string_product,
+				   dev->product);
 #ifdef ALLOW_SERIAL_NUMBER
 	if (dev->serial)
-		start += sprintf(start, format_string_serialnumber,
-				 dev->serial);
+		start += scnprintf(start, end - start, format_string_serialnumber,
+				   dev->serial);
 #endif
- out:
 	return start;
 }
 
@@ -369,19 +344,11 @@  static char *usb_dump_desc(char *start,
 {
 	int i;
 
-	if (start > end)
-		return start;
-
 	start = usb_dump_device_descriptor(start, end, &dev->descriptor);
 
-	if (start > end)
-		return start;
-
 	start = usb_dump_device_strings(start, end, dev);
 
 	for (i = 0; i < dev->descriptor.bNumConfigurations; i++) {
-		if (start > end)
-			return start;
 		start = usb_dump_config(dev->speed,
 				start, end, dev->config + i,
 				/* active ? */
@@ -479,11 +446,14 @@  static ssize_t usb_device_dump(char __us
 				bus->bandwidth_isoc_reqs);
 
 	}
-	data_end = usb_dump_desc(data_end, pages_start + (2 * PAGE_SIZE) - 256,
-				 usbdev);
+	data_end = usb_dump_desc(data_end, pages_start + 2 * PAGE_SIZE, usbdev);
 
-	if (data_end > (pages_start + (2 * PAGE_SIZE) - 256))
-		data_end += sprintf(data_end, "(truncated)\n");
+	/*
+	 * Iff our buffer is full, roll back a possibly truncated line at
+	 * the end; strrchr() call below can not ever return NULL...
+	 */
+	if (data_end >= pages_start + 2 * PAGE_SIZE - 1)
+		data_end = strrchr(pages_start, '\n') + 1;
 
 	length = data_end - pages_start;
 	/* if we can start copying some data to the user */