| Message ID | 20250116160543.216913-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 70cd0576aa39c55aabd227851cba0c601e811fb6 |
| Headers | show |
| Series | [v1,1/1] usb: hcd: Bump local buffer size in rh_string() | expand |
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
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?)
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
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?
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.
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}; >
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};
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(-)