[PATCH v1 1/1] usb: hcd: Bump local buffer size in rh_string()

[PATCH v1 1/1] usb: hcd: Bump local buffer size in rh_string()

[Andy Shevchenko]

GCC is not happy about the buffer size:

drivers/usb/core/hcd.c:441:48: error: ‘%s’ directive output may be truncated writing up to 64 bytes into a region of size between 35 and 99 [-Werror=format-truncation=]
  441 |                 snprintf (buf, sizeof buf, "%s %s %s", init_utsname()->sysname,
      |                                                ^~
  442 |                         init_utsname()->release, hcd->driver->description);
      |                         ~~~~~~~~~~~~~~~~~~~~~~~

Bump the size to get it enough for the possible strings.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/usb/core/hcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 0b2490347b9f..a75cf1f6d741 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -415,7 +415,7 @@ ascii2desc(char const *s, u8 *buf, unsigned len)
 static unsigned
 rh_string(int id, struct usb_hcd const *hcd, u8 *data, unsigned len)
 {
-	char buf[100];
+	char buf[160];
 	char const *s;
 	static char const langids[4] = {4, USB_DT_STRING, 0x09, 0x04};
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 1/1] usb: hcd: Bump local buffer size in rh_string()

[Greg Kroah-Hartman]

On Thu, Jan 16, 2025 at 06:05:43PM +0200, Andy Shevchenko wrote:
> GCC is not happy about the buffer size:
> 
> drivers/usb/core/hcd.c:441:48: error: ‘%s’ directive output may be truncated writing up to 64 bytes into a region of size between 35 and 99 [-Werror=format-truncation=]
>   441 |                 snprintf (buf, sizeof buf, "%s %s %s", init_utsname()->sysname,
>       |                                                ^~
>   442 |                         init_utsname()->release, hcd->driver->description);
>       |                         ~~~~~~~~~~~~~~~~~~~~~~~
> 
> Bump the size to get it enough for the possible strings.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/usb/core/hcd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 0b2490347b9f..a75cf1f6d741 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -415,7 +415,7 @@ ascii2desc(char const *s, u8 *buf, unsigned len)
>  static unsigned
>  rh_string(int id, struct usb_hcd const *hcd, u8 *data, unsigned len)
>  {
> -	char buf[100];
> +	char buf[160];
>  	char const *s;
>  	static char const langids[4] = {4, USB_DT_STRING, 0x09, 0x04};

Worst case it's properly truncated so why do we need to worry about this
"warning"?  And what compiler version is giving that, I don't see that
here in my build testing.

thanks,

greg k-h

Re: [PATCH v1 1/1] usb: hcd: Bump local buffer size in rh_string()

[Andy Shevchenko]

On Fri, Jan 17, 2025 at 07:11:46AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Jan 16, 2025 at 06:05:43PM +0200, Andy Shevchenko wrote:
> > GCC is not happy about the buffer size:
> > 
> > drivers/usb/core/hcd.c:441:48: error: ‘%s’ directive output may be truncated writing up to 64 bytes into a region of size between 35 and 99 [-Werror=format-truncation=]
> >   441 |                 snprintf (buf, sizeof buf, "%s %s %s", init_utsname()->sysname,
> >       |                                                ^~
> >   442 |                         init_utsname()->release, hcd->driver->description);
> >       |                         ~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > Bump the size to get it enough for the possible strings.

...

> >  static unsigned
> >  rh_string(int id, struct usb_hcd const *hcd, u8 *data, unsigned len)
> >  {
> > -	char buf[100];
> > +	char buf[160];
> >  	char const *s;
> >  	static char const langids[4] = {4, USB_DT_STRING, 0x09, 0x04};
> 
> Worst case it's properly truncated so why do we need to worry about this
> "warning"?

With CONFIG_WERROR=y it's a compilation error. My goal is to have
i386_defconfig and x86_64_defconfig to be compiled with `make W=1`.

> And what compiler version is giving that, I don't see that
> here in my build testing.

`make W=1` (and be sure that CONFIG_WERROR=y).

$ gcc --version
gcc (Debian 14.2.0-12) 14.2.0

(IIRC that had been started with any GCC from v13?)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] usb: hcd: Bump local buffer size in rh_string()

[Greg Kroah-Hartman]

On Fri, Jan 17, 2025 at 03:42:27PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 17, 2025 at 07:11:46AM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Jan 16, 2025 at 06:05:43PM +0200, Andy Shevchenko wrote:
> > > GCC is not happy about the buffer size:
> > > 
> > > drivers/usb/core/hcd.c:441:48: error: ‘%s’ directive output may be truncated writing up to 64 bytes into a region of size between 35 and 99 [-Werror=format-truncation=]
> > >   441 |                 snprintf (buf, sizeof buf, "%s %s %s", init_utsname()->sysname,
> > >       |                                                ^~
> > >   442 |                         init_utsname()->release, hcd->driver->description);
> > >       |                         ~~~~~~~~~~~~~~~~~~~~~~~
> > > 
> > > Bump the size to get it enough for the possible strings.
> 
> ...
> 
> > >  static unsigned
> > >  rh_string(int id, struct usb_hcd const *hcd, u8 *data, unsigned len)
> > >  {
> > > -	char buf[100];
> > > +	char buf[160];
> > >  	char const *s;
> > >  	static char const langids[4] = {4, USB_DT_STRING, 0x09, 0x04};
> > 
> > Worst case it's properly truncated so why do we need to worry about this
> > "warning"?
> 
> With CONFIG_WERROR=y it's a compilation error. My goal is to have
> i386_defconfig and x86_64_defconfig to be compiled with `make W=1`.

So you have to have W=1 enabled, right?  On my normal builds, with
CONFIG_WERROR=y enabled, I do not see this.

> > And what compiler version is giving that, I don't see that
> > here in my build testing.
> 
> `make W=1` (and be sure that CONFIG_WERROR=y).

Ah, ok, manual work here.

And I guess the error is right, ->sysname could be 64 and release can
also be 64 bytes long, so it would be truncated.

thanks,

greg k-h

Re: [PATCH v1 1/1] usb: hcd: Bump local buffer size in rh_string()

[Andy Shevchenko]

On Fri, Jan 17, 2025 at 03:26:13PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Jan 17, 2025 at 03:42:27PM +0200, Andy Shevchenko wrote:
> > On Fri, Jan 17, 2025 at 07:11:46AM +0100, Greg Kroah-Hartman wrote:
> > > On Thu, Jan 16, 2025 at 06:05:43PM +0200, Andy Shevchenko wrote:
> > > > GCC is not happy about the buffer size:
> > > > 
> > > > drivers/usb/core/hcd.c:441:48: error: ‘%s’ directive output may be truncated writing up to 64 bytes into a region of size between 35 and 99 [-Werror=format-truncation=]
> > > >   441 |                 snprintf (buf, sizeof buf, "%s %s %s", init_utsname()->sysname,
> > > >       |                                                ^~
> > > >   442 |                         init_utsname()->release, hcd->driver->description);
> > > >       |                         ~~~~~~~~~~~~~~~~~~~~~~~
> > > > 
> > > > Bump the size to get it enough for the possible strings.

...

> > > >  static unsigned
> > > >  rh_string(int id, struct usb_hcd const *hcd, u8 *data, unsigned len)
> > > >  {
> > > > -	char buf[100];
> > > > +	char buf[160];
> > > >  	char const *s;
> > > >  	static char const langids[4] = {4, USB_DT_STRING, 0x09, 0x04};
> > > 
> > > Worst case it's properly truncated so why do we need to worry about this
> > > "warning"?
> > 
> > With CONFIG_WERROR=y it's a compilation error. My goal is to have
> > i386_defconfig and x86_64_defconfig to be compiled with `make W=1`.
> 
> So you have to have W=1 enabled, right?

Yep!

> On my normal builds, with CONFIG_WERROR=y enabled, I do not see this.
> 
> > > And what compiler version is giving that, I don't see that
> > > here in my build testing.
> > 
> > `make W=1` (and be sure that CONFIG_WERROR=y).
> 
> Ah, ok, manual work here.
> 
> And I guess the error is right, ->sysname could be 64 and release can
> also be 64 bytes long, so it would be truncated.

Yeah... Should I update the commit message and issue v2?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] usb: hcd: Bump local buffer size in rh_string()

[Greg Kroah-Hartman]

On Fri, Jan 17, 2025 at 04:33:03PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 17, 2025 at 03:26:13PM +0100, Greg Kroah-Hartman wrote:
> > On Fri, Jan 17, 2025 at 03:42:27PM +0200, Andy Shevchenko wrote:
> > > On Fri, Jan 17, 2025 at 07:11:46AM +0100, Greg Kroah-Hartman wrote:
> > > > On Thu, Jan 16, 2025 at 06:05:43PM +0200, Andy Shevchenko wrote:
> > > > > GCC is not happy about the buffer size:
> > > > > 
> > > > > drivers/usb/core/hcd.c:441:48: error: ‘%s’ directive output may be truncated writing up to 64 bytes into a region of size between 35 and 99 [-Werror=format-truncation=]
> > > > >   441 |                 snprintf (buf, sizeof buf, "%s %s %s", init_utsname()->sysname,
> > > > >       |                                                ^~
> > > > >   442 |                         init_utsname()->release, hcd->driver->description);
> > > > >       |                         ~~~~~~~~~~~~~~~~~~~~~~~
> > > > > 
> > > > > Bump the size to get it enough for the possible strings.
> 
> ...
> 
> > > > >  static unsigned
> > > > >  rh_string(int id, struct usb_hcd const *hcd, u8 *data, unsigned len)
> > > > >  {
> > > > > -	char buf[100];
> > > > > +	char buf[160];
> > > > >  	char const *s;
> > > > >  	static char const langids[4] = {4, USB_DT_STRING, 0x09, 0x04};
> > > > 
> > > > Worst case it's properly truncated so why do we need to worry about this
> > > > "warning"?
> > > 
> > > With CONFIG_WERROR=y it's a compilation error. My goal is to have
> > > i386_defconfig and x86_64_defconfig to be compiled with `make W=1`.
> > 
> > So you have to have W=1 enabled, right?
> 
> Yep!
> 
> > On my normal builds, with CONFIG_WERROR=y enabled, I do not see this.
> > 
> > > > And what compiler version is giving that, I don't see that
> > > > here in my build testing.
> > > 
> > > `make W=1` (and be sure that CONFIG_WERROR=y).
> > 
> > Ah, ok, manual work here.
> > 
> > And I guess the error is right, ->sysname could be 64 and release can
> > also be 64 bytes long, so it would be truncated.
> 
> Yeah... Should I update the commit message and issue v2?

No need, I've already take this as-is.

Re: [PATCH v1 1/1] usb: hcd: Bump local buffer size in rh_string()

[David Laight]

On Thu, 16 Jan 2025 18:05:43 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> GCC is not happy about the buffer size:
> 
> drivers/usb/core/hcd.c:441:48: error: ‘%s’ directive output may be truncated writing up to 64 bytes into a region of size between 35 and 99 [-Werror=format-truncation=]
>   441 |                 snprintf (buf, sizeof buf, "%s %s %s", init_utsname()->sysname,
>       |                                                ^~
>   442 |                         init_utsname()->release, hcd->driver->description);
>       |                         ~~~~~~~~~~~~~~~~~~~~~~~
> 
> Bump the size to get it enough for the possible strings.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/usb/core/hcd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 0b2490347b9f..a75cf1f6d741 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -415,7 +415,7 @@ ascii2desc(char const *s, u8 *buf, unsigned len)
>  static unsigned
>  rh_string(int id, struct usb_hcd const *hcd, u8 *data, unsigned len)
>  {
> -	char buf[100];
> +	char buf[160];

Pretty pointless - look at ascii2desc() just above.
(Converts to LE i6-bit chars with a leading type+length.)
It gets truncated to 126 characters.
Indeed the entire snprintf() is pretty pointless given what happens to the
data given that it is all strings.

Is the overall truncation even right?
The outer length is bounded to 254, but there may be fewer characters in the
buffer because the buffer length itself might be smaller.
Seems a recipe for disaster.

	David 


>  	char const *s;
>  	static char const langids[4] = {4, USB_DT_STRING, 0x09, 0x04};
>