[PATCH] usb: core: use sysfs_emit() instead of sprintf()

[PATCH] usb: core: use sysfs_emit() instead of sprintf()

[Sergey Shtylyov]

sprintf() (still used in the USB core for the sysfs output) is vulnerable
to the buffer overflow.  Use the new-fangled sysfs_emit() instead.

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.

 drivers/usb/core/endpoint.c        |   11 ++---
 drivers/usb/core/ledtrig-usbport.c |    3 -
 drivers/usb/core/port.c            |   11 ++---
 drivers/usb/core/sysfs.c           |   77 ++++++++++++++++++-------------------
 4 files changed, 53 insertions(+), 49 deletions(-)

Index: usb/drivers/usb/core/endpoint.c
===================================================================
--- usb.orig/drivers/usb/core/endpoint.c
+++ usb/drivers/usb/core/endpoint.c
@@ -14,6 +14,7 @@
 #include <linux/kernel.h>
 #include <linux/spinlock.h>
 #include <linux/slab.h>
+#include <linux/sysfs.h>
 #include <linux/usb.h>
 #include "usb.h"
 
@@ -39,7 +40,7 @@ static ssize_t field##_show(struct devic
 			       char *buf)			\
 {								\
 	struct ep_device *ep = to_ep_device(dev);		\
-	return sprintf(buf, format_string, ep->desc->field);	\
+	return sysfs_emit(buf, format_string, ep->desc->field);	\
 }								\
 static DEVICE_ATTR_RO(field)
 
@@ -52,7 +53,7 @@ static ssize_t wMaxPacketSize_show(struc
 				   struct device_attribute *attr, char *buf)
 {
 	struct ep_device *ep = to_ep_device(dev);
-	return sprintf(buf, "%04x\n", usb_endpoint_maxp(ep->desc));
+	return sysfs_emit(buf, "%04x\n", usb_endpoint_maxp(ep->desc));
 }
 static DEVICE_ATTR_RO(wMaxPacketSize);
 
@@ -76,7 +77,7 @@ static ssize_t type_show(struct device *
 		type = "Interrupt";
 		break;
 	}
-	return sprintf(buf, "%s\n", type);
+	return sysfs_emit(buf, "%s\n", type);
 }
 static DEVICE_ATTR_RO(type);
 
@@ -95,7 +96,7 @@ static ssize_t interval_show(struct devi
 		interval /= 1000;
 	}
 
-	return sprintf(buf, "%d%cs\n", interval, unit);
+	return sysfs_emit(buf, "%d%cs\n", interval, unit);
 }
 static DEVICE_ATTR_RO(interval);
 
@@ -111,7 +112,7 @@ static ssize_t direction_show(struct dev
 		direction = "in";
 	else
 		direction = "out";
-	return sprintf(buf, "%s\n", direction);
+	return sysfs_emit(buf, "%s\n", direction);
 }
 static DEVICE_ATTR_RO(direction);
 
Index: usb/drivers/usb/core/ledtrig-usbport.c
===================================================================
--- usb.orig/drivers/usb/core/ledtrig-usbport.c
+++ usb/drivers/usb/core/ledtrig-usbport.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/slab.h>
+#include <linux/sysfs.h>
 #include <linux/usb.h>
 #include <linux/usb/of.h>
 
@@ -87,7 +88,7 @@ static ssize_t usbport_trig_port_show(st
 						      struct usbport_trig_port,
 						      attr);
 
-	return sprintf(buf, "%d\n", port->observed) + 1;
+	return sysfs_emit(buf, "%d\n", port->observed) + 1;
 }
 
 static ssize_t usbport_trig_port_store(struct device *dev,
Index: usb/drivers/usb/core/port.c
===================================================================
--- usb.orig/drivers/usb/core/port.c
+++ usb/drivers/usb/core/port.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/slab.h>
+#include <linux/sysfs.h>
 #include <linux/pm_qos.h>
 #include <linux/component.h>
 
@@ -22,7 +23,7 @@ static ssize_t location_show(struct devi
 {
 	struct usb_port *port_dev = to_usb_port(dev);
 
-	return sprintf(buf, "0x%08x\n", port_dev->location);
+	return sysfs_emit(buf, "0x%08x\n", port_dev->location);
 }
 static DEVICE_ATTR_RO(location);
 
@@ -47,7 +48,7 @@ static ssize_t connect_type_show(struct
 		break;
 	}
 
-	return sprintf(buf, "%s\n", result);
+	return sysfs_emit(buf, "%s\n", result);
 }
 static DEVICE_ATTR_RO(connect_type);
 
@@ -56,7 +57,7 @@ static ssize_t over_current_count_show(s
 {
 	struct usb_port *port_dev = to_usb_port(dev);
 
-	return sprintf(buf, "%u\n", port_dev->over_current_count);
+	return sysfs_emit(buf, "%u\n", port_dev->over_current_count);
 }
 static DEVICE_ATTR_RO(over_current_count);
 
@@ -65,7 +66,7 @@ static ssize_t quirks_show(struct device
 {
 	struct usb_port *port_dev = to_usb_port(dev);
 
-	return sprintf(buf, "%08x\n", port_dev->quirks);
+	return sysfs_emit(buf, "%08x\n", port_dev->quirks);
 }
 
 static ssize_t quirks_store(struct device *dev, struct device_attribute *attr,
@@ -100,7 +101,7 @@ static ssize_t usb3_lpm_permit_show(stru
 			p = "0";
 	}
 
-	return sprintf(buf, "%s\n", p);
+	return sysfs_emit(buf, "%s\n", p);
 }
 
 static ssize_t usb3_lpm_permit_store(struct device *dev,
Index: usb/drivers/usb/core/sysfs.c
===================================================================
--- usb.orig/drivers/usb/core/sysfs.c
+++ usb/drivers/usb/core/sysfs.c
@@ -14,6 +14,7 @@
 
 #include <linux/kernel.h>
 #include <linux/string.h>
+#include <linux/sysfs.h>
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
 #include <linux/usb/quirks.h>
@@ -35,7 +36,7 @@ static ssize_t field##_show(struct devic
 		return -EINTR;						\
 	actconfig = udev->actconfig;					\
 	if (actconfig)							\
-		rc = sprintf(buf, format_string,			\
+		rc = sysfs_emit(buf, format_string,			\
 				actconfig->desc.field);			\
 	usb_unlock_device(udev);					\
 	return rc;							\
@@ -61,7 +62,8 @@ static ssize_t bMaxPower_show(struct dev
 		return -EINTR;
 	actconfig = udev->actconfig;
 	if (actconfig)
-		rc = sprintf(buf, "%dmA\n", usb_get_max_power(udev, actconfig));
+		rc = sysfs_emit(buf, "%dmA\n",
+				usb_get_max_power(udev, actconfig));
 	usb_unlock_device(udev);
 	return rc;
 }
@@ -80,7 +82,7 @@ static ssize_t configuration_show(struct
 		return -EINTR;
 	actconfig = udev->actconfig;
 	if (actconfig && actconfig->string)
-		rc = sprintf(buf, "%s\n", actconfig->string);
+		rc = sysfs_emit(buf, "%s\n", actconfig->string);
 	usb_unlock_device(udev);
 	return rc;
 }
@@ -114,7 +116,7 @@ static ssize_t devspec_show(struct devic
 {
 	struct device_node *of_node = dev->of_node;
 
-	return sprintf(buf, "%pOF\n", of_node);
+	return sysfs_emit(buf, "%pOF\n", of_node);
 }
 static DEVICE_ATTR_RO(devspec);
 #endif
@@ -131,7 +133,7 @@ static ssize_t  name##_show(struct devic
 	retval = usb_lock_device_interruptible(udev);			\
 	if (retval < 0)							\
 		return -EINTR;						\
-	retval = sprintf(buf, "%s\n", udev->name);			\
+	retval = sysfs_emit(buf, "%s\n", udev->name);			\
 	usb_unlock_device(udev);					\
 	return retval;							\
 }									\
@@ -175,7 +177,7 @@ static ssize_t speed_show(struct device
 	default:
 		speed = "unknown";
 	}
-	return sprintf(buf, "%s\n", speed);
+	return sysfs_emit(buf, "%s\n", speed);
 }
 static DEVICE_ATTR_RO(speed);
 
@@ -185,7 +187,7 @@ static ssize_t rx_lanes_show(struct devi
 	struct usb_device *udev;
 
 	udev = to_usb_device(dev);
-	return sprintf(buf, "%d\n", udev->rx_lanes);
+	return sysfs_emit(buf, "%d\n", udev->rx_lanes);
 }
 static DEVICE_ATTR_RO(rx_lanes);
 
@@ -195,7 +197,7 @@ static ssize_t tx_lanes_show(struct devi
 	struct usb_device *udev;
 
 	udev = to_usb_device(dev);
-	return sprintf(buf, "%d\n", udev->tx_lanes);
+	return sysfs_emit(buf, "%d\n", udev->tx_lanes);
 }
 static DEVICE_ATTR_RO(tx_lanes);
 
@@ -205,7 +207,7 @@ static ssize_t busnum_show(struct device
 	struct usb_device *udev;
 
 	udev = to_usb_device(dev);
-	return sprintf(buf, "%d\n", udev->bus->busnum);
+	return sysfs_emit(buf, "%d\n", udev->bus->busnum);
 }
 static DEVICE_ATTR_RO(busnum);
 
@@ -215,7 +217,7 @@ static ssize_t devnum_show(struct device
 	struct usb_device *udev;
 
 	udev = to_usb_device(dev);
-	return sprintf(buf, "%d\n", udev->devnum);
+	return sysfs_emit(buf, "%d\n", udev->devnum);
 }
 static DEVICE_ATTR_RO(devnum);
 
@@ -225,7 +227,7 @@ static ssize_t devpath_show(struct devic
 	struct usb_device *udev;
 
 	udev = to_usb_device(dev);
-	return sprintf(buf, "%s\n", udev->devpath);
+	return sysfs_emit(buf, "%s\n", udev->devpath);
 }
 static DEVICE_ATTR_RO(devpath);
 
@@ -237,7 +239,7 @@ static ssize_t version_show(struct devic
 
 	udev = to_usb_device(dev);
 	bcdUSB = le16_to_cpu(udev->descriptor.bcdUSB);
-	return sprintf(buf, "%2x.%02x\n", bcdUSB >> 8, bcdUSB & 0xff);
+	return sysfs_emit(buf, "%2x.%02x\n", bcdUSB >> 8, bcdUSB & 0xff);
 }
 static DEVICE_ATTR_RO(version);
 
@@ -247,7 +249,7 @@ static ssize_t maxchild_show(struct devi
 	struct usb_device *udev;
 
 	udev = to_usb_device(dev);
-	return sprintf(buf, "%d\n", udev->maxchild);
+	return sysfs_emit(buf, "%d\n", udev->maxchild);
 }
 static DEVICE_ATTR_RO(maxchild);
 
@@ -257,7 +259,7 @@ static ssize_t quirks_show(struct device
 	struct usb_device *udev;
 
 	udev = to_usb_device(dev);
-	return sprintf(buf, "0x%x\n", udev->quirks);
+	return sysfs_emit(buf, "0x%x\n", udev->quirks);
 }
 static DEVICE_ATTR_RO(quirks);
 
@@ -267,7 +269,7 @@ static ssize_t avoid_reset_quirk_show(st
 	struct usb_device *udev;
 
 	udev = to_usb_device(dev);
-	return sprintf(buf, "%d\n", !!(udev->quirks & USB_QUIRK_RESET));
+	return sysfs_emit(buf, "%d\n", !!(udev->quirks & USB_QUIRK_RESET));
 }
 
 static ssize_t avoid_reset_quirk_store(struct device *dev,
@@ -297,7 +299,7 @@ static ssize_t urbnum_show(struct device
 	struct usb_device *udev;
 
 	udev = to_usb_device(dev);
-	return sprintf(buf, "%d\n", atomic_read(&udev->urbnum));
+	return sysfs_emit(buf, "%d\n", atomic_read(&udev->urbnum));
 }
 static DEVICE_ATTR_RO(urbnum);
 
@@ -305,8 +307,8 @@ static ssize_t ltm_capable_show(struct d
 				struct device_attribute *attr, char *buf)
 {
 	if (usb_device_supports_ltm(to_usb_device(dev)))
-		return sprintf(buf, "%s\n", "yes");
-	return sprintf(buf, "%s\n", "no");
+		return sysfs_emit(buf, "%s\n", "yes");
+	return sysfs_emit(buf, "%s\n", "no");
 }
 static DEVICE_ATTR_RO(ltm_capable);
 
@@ -317,7 +319,7 @@ static ssize_t persist_show(struct devic
 {
 	struct usb_device *udev = to_usb_device(dev);
 
-	return sprintf(buf, "%d\n", udev->persist_enabled);
+	return sysfs_emit(buf, "%d\n", udev->persist_enabled);
 }
 
 static ssize_t persist_store(struct device *dev, struct device_attribute *attr,
@@ -372,7 +374,7 @@ static ssize_t connected_duration_show(s
 {
 	struct usb_device *udev = to_usb_device(dev);
 
-	return sprintf(buf, "%u\n",
+	return sysfs_emit(buf, "%u\n",
 			jiffies_to_msecs(jiffies - udev->connect_time));
 }
 static DEVICE_ATTR_RO(connected_duration);
@@ -394,14 +396,14 @@ static ssize_t active_duration_show(stru
 		duration = jiffies_to_msecs(jiffies + udev->active_duration);
 	else
 		duration = jiffies_to_msecs(udev->active_duration);
-	return sprintf(buf, "%u\n", duration);
+	return sysfs_emit(buf, "%u\n", duration);
 }
 static DEVICE_ATTR_RO(active_duration);
 
 static ssize_t autosuspend_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%d\n", dev->power.autosuspend_delay / 1000);
+	return sysfs_emit(buf, "%d\n", dev->power.autosuspend_delay / 1000);
 }
 
 static ssize_t autosuspend_store(struct device *dev,
@@ -442,7 +444,7 @@ static ssize_t level_show(struct device
 	warn_level();
 	if (udev->state != USB_STATE_SUSPENDED && !udev->dev.power.runtime_auto)
 		p = on_string;
-	return sprintf(buf, "%s\n", p);
+	return sysfs_emit(buf, "%s\n", p);
 }
 
 static ssize_t level_store(struct device *dev, struct device_attribute *attr,
@@ -490,7 +492,7 @@ static ssize_t usb2_hardware_lpm_show(st
 	else
 		p = "disabled";
 
-	return sprintf(buf, "%s\n", p);
+	return sysfs_emit(buf, "%s\n", p);
 }
 
 static ssize_t usb2_hardware_lpm_store(struct device *dev,
@@ -529,7 +531,7 @@ static ssize_t usb2_lpm_l1_timeout_show(
 					char *buf)
 {
 	struct usb_device *udev = to_usb_device(dev);
-	return sprintf(buf, "%d\n", udev->l1_params.timeout);
+	return sysfs_emit(buf, "%d\n", udev->l1_params.timeout);
 }
 
 static ssize_t usb2_lpm_l1_timeout_store(struct device *dev,
@@ -552,7 +554,7 @@ static ssize_t usb2_lpm_besl_show(struct
 				  struct device_attribute *attr, char *buf)
 {
 	struct usb_device *udev = to_usb_device(dev);
-	return sprintf(buf, "%d\n", udev->l1_params.besl);
+	return sysfs_emit(buf, "%d\n", udev->l1_params.besl);
 }
 
 static ssize_t usb2_lpm_besl_store(struct device *dev,
@@ -589,7 +591,7 @@ static ssize_t usb3_hardware_lpm_u1_show
 
 	usb_unlock_device(udev);
 
-	return sprintf(buf, "%s\n", p);
+	return sysfs_emit(buf, "%s\n", p);
 }
 static DEVICE_ATTR_RO(usb3_hardware_lpm_u1);
 
@@ -611,7 +613,7 @@ static ssize_t usb3_hardware_lpm_u2_show
 
 	usb_unlock_device(udev);
 
-	return sprintf(buf, "%s\n", p);
+	return sysfs_emit(buf, "%s\n", p);
 }
 static DEVICE_ATTR_RO(usb3_hardware_lpm_u2);
 
@@ -694,7 +696,7 @@ field##_show(struct device *dev, struct
 	struct usb_device *udev;					\
 									\
 	udev = to_usb_device(dev);					\
-	return sprintf(buf, format_string, 				\
+	return sysfs_emit(buf, format_string,				\
 			le16_to_cpu(udev->descriptor.field));		\
 }									\
 static DEVICE_ATTR_RO(field)
@@ -711,7 +713,7 @@ field##_show(struct device *dev, struct
 	struct usb_device *udev;					\
 									\
 	udev = to_usb_device(dev);					\
-	return sprintf(buf, format_string, udev->descriptor.field);	\
+	return sysfs_emit(buf, format_string, udev->descriptor.field);	\
 }									\
 static DEVICE_ATTR_RO(field)
 
@@ -957,7 +959,7 @@ static ssize_t interface_authorized_defa
 	struct usb_device *usb_dev = to_usb_device(dev);
 	struct usb_hcd *hcd = bus_to_hcd(usb_dev->bus);
 
-	return sprintf(buf, "%u\n", !!HCD_INTF_AUTHORIZED(hcd));
+	return sysfs_emit(buf, "%u\n", !!HCD_INTF_AUTHORIZED(hcd));
 }
 
 /*
@@ -1066,7 +1068,7 @@ iad_##field##_show(struct device *dev, s
 {									\
 	struct usb_interface *intf = to_usb_interface(dev);		\
 									\
-	return sprintf(buf, format_string,				\
+	return sysfs_emit(buf, format_string,				\
 			intf->intf_assoc->field); 			\
 }									\
 static DEVICE_ATTR_RO(iad_##field)
@@ -1085,7 +1087,7 @@ field##_show(struct device *dev, struct
 {									\
 	struct usb_interface *intf = to_usb_interface(dev);		\
 									\
-	return sprintf(buf, format_string,				\
+	return sysfs_emit(buf, format_string,				\
 			intf->cur_altsetting->desc.field); 		\
 }									\
 static DEVICE_ATTR_RO(field)
@@ -1107,7 +1109,7 @@ static ssize_t interface_show(struct dev
 	string = READ_ONCE(intf->cur_altsetting->string);
 	if (!string)
 		return 0;
-	return sprintf(buf, "%s\n", string);
+	return sysfs_emit(buf, "%s\n", string);
 }
 static DEVICE_ATTR_RO(interface);
 
@@ -1122,8 +1124,7 @@ static ssize_t modalias_show(struct devi
 	udev = interface_to_usbdev(intf);
 	alt = READ_ONCE(intf->cur_altsetting);
 
-	return sprintf(buf, "usb:v%04Xp%04Xd%04Xdc%02Xdsc%02Xdp%02X"
-			"ic%02Xisc%02Xip%02Xin%02X\n",
+	return sysfs_emit(buf, "usb:v%04Xp%04Xd%04Xdc%02Xdsc%02Xdp%02Xic%02Xisc%02Xip%02Xin%02X\n",
 			le16_to_cpu(udev->descriptor.idVendor),
 			le16_to_cpu(udev->descriptor.idProduct),
 			le16_to_cpu(udev->descriptor.bcdDevice),
@@ -1150,7 +1151,7 @@ static ssize_t supports_autosuspend_show
 	s = (!dev->driver || to_usb_driver(dev->driver)->supports_autosuspend);
 	device_unlock(dev);
 
-	return sprintf(buf, "%u\n", s);
+	return sysfs_emit(buf, "%u\n", s);
 }
 static DEVICE_ATTR_RO(supports_autosuspend);
 
@@ -1163,7 +1164,7 @@ static ssize_t interface_authorized_show
 {
 	struct usb_interface *intf = to_usb_interface(dev);
 
-	return sprintf(buf, "%u\n", intf->authorized);
+	return sysfs_emit(buf, "%u\n", intf->authorized);
 }
 
 /*

Re: [PATCH] usb: core: use sysfs_emit() instead of sprintf()

[Greg Kroah-Hartman]

On Tue, Feb 08, 2022 at 02:02:20PM +0300, Sergey Shtylyov wrote:
> sprintf() (still used in the USB core for the sysfs output) is vulnerable
> to the buffer overflow.

Really?  Where?  If we have potential overflows, let's fix them as bug
fixes and properly backport the fixes where needed.

If these really are just using the "old-style" functions instead, then
that's something totally different and you should not say "vulnerable"
if it really is not at all.

> Use the new-fangled sysfs_emit() instead.
> 
> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> analysis tool.

You mean coccinelle, right?  It's been checking for this for a while.

Also properly wrap your changelog at 72 columns please.

thanks,

greg k-h

Re: [PATCH] usb: core: use sysfs_emit() instead of sprintf()

[Sergey Shtylyov]

On 2/8/22 2:21 PM, Greg Kroah-Hartman wrote:

>> sprintf() (still used in the USB core for the sysfs output) is vulnerable
>> to the buffer overflow.
> 
> Really?  Where?  If we have potential overflows, let's fix them as bug
> fixes and properly backport the fixes where needed.

   I must admit I didn't found any real overflows in my quick triage...

> If these really are just using the "old-style" functions instead, then
> that's something totally different and you should not say "vulnerable"
> if it really is not at all.

   Isn't sprint() generally considered harmful? :-)

>> Use the new-fangled sysfs_emit() instead.
>>
>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
>> analysis tool.
> 
> You mean coccinelle, right?

   Do you think coccinelle is the only code analyzer in this world? :-)
   I told you I was using SVACE (made by Russian Institute of the System Programming).

>  It's been checking for this for a while.
> 
> Also properly wrap your changelog at 72 columns please.

   Well, checkpatch.pl was content. :-)

> thanks,
> 
> greg k-h

MBR, Sergey

Re: [PATCH] usb: core: use sysfs_emit() instead of sprintf()

[Greg Kroah-Hartman]

On Tue, Feb 08, 2022 at 02:49:37PM +0300, Sergey Shtylyov wrote:
> On 2/8/22 2:21 PM, Greg Kroah-Hartman wrote:
> 
> >> sprintf() (still used in the USB core for the sysfs output) is vulnerable
> >> to the buffer overflow.
> > 
> > Really?  Where?  If we have potential overflows, let's fix them as bug
> > fixes and properly backport the fixes where needed.
> 
>    I must admit I didn't found any real overflows in my quick triage...

Then please do not scare people by saying otherwise.

> 
> > If these really are just using the "old-style" functions instead, then
> > that's something totally different and you should not say "vulnerable"
> > if it really is not at all.
> 
>    Isn't sprint() generally considered harmful? :-)

For sysfs files that have a known size (PAGE_SIZE) with a single value
like this, no, it's not harmful.

> >> Use the new-fangled sysfs_emit() instead.
> >>
> >> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> >> analysis tool.
> > 
> > You mean coccinelle, right?
> 
>    Do you think coccinelle is the only code analyzer in this world? :-)

No, but it has a built-in rule for this already, why not just use that
to find these types of things?

>    I told you I was using SVACE (made by Russian Institute of the System Programming).

Nice, where is the rule for this with that tool?

thanks,

greg k-h