| Message ID | 20250321164949.423957-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/1] usb: Add checks for snprintf() calls in usb_alloc_dev() | expand |
On Fri, Mar 21, 2025 at 06:49:49PM +0200, Andy Shevchenko wrote: > When creating a device path in the driver the snprintf() takes > up to 16 characters long argument along with the additional up to > 12 characters for the signed integer (as it can't see the actual limits) > and tries to pack this into 16 bytes array. GCC complains about that > when build with `make W=1`: > > drivers/usb/core/usb.c:705:25: note: ‘snprintf’ output between 3 and 28 bytes into a destination of size 16 > > Since everything works until now, let's just check for the potential > buffer overflow and bail out. It is most likely a never happen situation, > but at least it makes GCC happy. Any comments anybody?
On Sat, Apr 19, 2025 at 07:27:08PM +0300, Andy Shevchenko wrote: > On Fri, Mar 21, 2025 at 06:49:49PM +0200, Andy Shevchenko wrote: > > When creating a device path in the driver the snprintf() takes > > up to 16 characters long argument along with the additional up to > > 12 characters for the signed integer (as it can't see the actual limits) > > and tries to pack this into 16 bytes array. GCC complains about that > > when build with `make W=1`: > > > > drivers/usb/core/usb.c:705:25: note: ‘snprintf’ output between 3 and 28 bytes into a destination of size 16 > > > > Since everything works until now, let's just check for the potential > > buffer overflow and bail out. It is most likely a never happen situation, > > but at least it makes GCC happy. > > Any comments anybody? It's not a hot path; the extra check won't hurt anything. Acked-by: Alan Stern <stern@rowland.harvard.edu> Alan Stern
On Sat, Apr 19, 2025 at 07:27:08PM +0300, Andy Shevchenko wrote: > On Fri, Mar 21, 2025 at 06:49:49PM +0200, Andy Shevchenko wrote: > > When creating a device path in the driver the snprintf() takes > > up to 16 characters long argument along with the additional up to > > 12 characters for the signed integer (as it can't see the actual limits) > > and tries to pack this into 16 bytes array. GCC complains about that > > when build with `make W=1`: > > > > drivers/usb/core/usb.c:705:25: note: ‘snprintf’ output between 3 and 28 bytes into a destination of size 16 > > > > Since everything works until now, let's just check for the potential > > buffer overflow and bail out. It is most likely a never happen situation, > > but at least it makes GCC happy. > > Any comments anybody? It's been added to my tree last week, why comment again? confused, greg k-h
Sun, Apr 20, 2025 at 08:27:59AM +0200, Greg Kroah-Hartman kirjoitti: > On Sat, Apr 19, 2025 at 07:27:08PM +0300, Andy Shevchenko wrote: > > On Fri, Mar 21, 2025 at 06:49:49PM +0200, Andy Shevchenko wrote: > > > When creating a device path in the driver the snprintf() takes > > > up to 16 characters long argument along with the additional up to > > > 12 characters for the signed integer (as it can't see the actual limits) > > > and tries to pack this into 16 bytes array. GCC complains about that > > > when build with `make W=1`: > > > > > > drivers/usb/core/usb.c:705:25: note: ‘snprintf’ output between 3 and 28 bytes into a destination of size 16 > > > > > > Since everything works until now, let's just check for the potential > > > buffer overflow and bail out. It is most likely a never happen situation, > > > but at least it makes GCC happy. > > > > Any comments anybody? > > It's been added to my tree last week, Thank you! > why comment again? Ah, I missed that, too many emails lately. :-( > confused,
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c index 0b4685aad2d5..118fa4c93a79 100644 --- a/drivers/usb/core/usb.c +++ b/drivers/usb/core/usb.c @@ -695,15 +695,16 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent, device_set_of_node_from_dev(&dev->dev, bus->sysdev); dev_set_name(&dev->dev, "usb%d", bus->busnum); } else { + int n; + /* match any labeling on the hubs; it's one-based */ if (parent->devpath[0] == '0') { - snprintf(dev->devpath, sizeof dev->devpath, - "%d", port1); + n = snprintf(dev->devpath, sizeof(dev->devpath), "%d", port1); /* Root ports are not counted in route string */ dev->route = 0; } else { - snprintf(dev->devpath, sizeof dev->devpath, - "%s.%d", parent->devpath, port1); + n = snprintf(dev->devpath, sizeof(dev->devpath), "%s.%d", + parent->devpath, port1); /* Route string assumes hubs have less than 16 ports */ if (port1 < 15) dev->route = parent->route + @@ -712,6 +713,11 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent, dev->route = parent->route + (15 << ((parent->level - 1)*4)); } + if (n >= sizeof(dev->devpath)) { + usb_put_hcd(bus_to_hcd(bus)); + usb_put_dev(dev); + return NULL; + } dev->dev.parent = &parent->dev; dev_set_name(&dev->dev, "%d-%s", bus->busnum, dev->devpath);
When creating a device path in the driver the snprintf() takes up to 16 characters long argument along with the additional up to 12 characters for the signed integer (as it can't see the actual limits) and tries to pack this into 16 bytes array. GCC complains about that when build with `make W=1`: drivers/usb/core/usb.c:705:25: note: ‘snprintf’ output between 3 and 28 bytes into a destination of size 16 Since everything works until now, let's just check for the potential buffer overflow and bail out. It is most likely a never happen situation, but at least it makes GCC happy. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/usb/core/usb.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)