| Message ID | 20171108120844.3196747-1-arnd@arndb.de |
|---|---|
| State | Accepted |
| Commit | 54d11736ec3179954b6cdea4b4e1136d6bb39f39 |
| Headers | show |
| Series | dell-smbios: fix string overflow | expand |
Hi, I recommend using "platform/x86: dell-smbios:" in commit header. On 11/08/2017 04:08 AM, Arnd Bergmann wrote: > The new sysfs code overwrites two fixed-length character arrays > that are each one byte shorter than they need to be, to hold > the trailing \0: > > drivers/platform/x86/dell-smbios.c: In function 'build_tokens_sysfs': > drivers/platform/x86/dell-smbios.c:494:42: error: 'sprintf' writing a terminating nul past the end of the destination [-Werror=format-overflow=] > sprintf(buffer_location, "%04x_location", > drivers/platform/x86/dell-smbios.c:494:3: note: 'sprintf' output 14 bytes into a destination of size 13 > drivers/platform/x86/dell-smbios.c:506:36: error: 'sprintf' writing a terminating nul past the end of the destination [-Werror=format-overflow=] > sprintf(buffer_value, "%04x_value", > drivers/platform/x86/dell-smbios.c:506:3: note: 'sprintf' output 11 bytes into a destination of size 10 Don't need to include the error log in commit message. Just explaining the issue is good enough. > > This changes it to just use kasprintf(), which always gets it right. > > Fixes: 33b9ca1e53b4 ("platform/x86: dell-smbios: Add a sysfs interface for SMBIOS tokens") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/platform/x86/dell-smbios.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c > index d99edd803c19..6a60db515bda 100644 > --- a/drivers/platform/x86/dell-smbios.c > +++ b/drivers/platform/x86/dell-smbios.c > @@ -463,8 +463,6 @@ static struct platform_driver platform_driver = { > > static int build_tokens_sysfs(struct platform_device *dev) > { > - char buffer_location[13]; > - char buffer_value[10]; > char *location_name; > char *value_name; > size_t size; > @@ -491,9 +489,8 @@ static int build_tokens_sysfs(struct platform_device *dev) > if (da_tokens[i].tokenID == 0) > continue; > /* add location */ > - sprintf(buffer_location, "%04x_location", > - da_tokens[i].tokenID); > - location_name = kstrdup(buffer_location, GFP_KERNEL); > + location_name = kasprintf(GFP_KERNEL, "%04x_location", > + da_tokens[i].tokenID); > if (location_name == NULL) > goto out_unwind_strings; > sysfs_attr_init(&token_location_attrs[i].attr); > @@ -503,9 +500,8 @@ static int build_tokens_sysfs(struct platform_device *dev) > token_attrs[j++] = &token_location_attrs[i].attr; > > /* add value */ > - sprintf(buffer_value, "%04x_value", > - da_tokens[i].tokenID); > - value_name = kstrdup(buffer_value, GFP_KERNEL); > + value_name = kasprintf(GFP_KERNEL, "%04x_value", > + da_tokens[i].tokenID); > if (value_name == NULL) > goto loop_fail_create_value; > sysfs_attr_init(&token_value_attrs[i].attr); -- Sathyanarayanan Kuppuswamy Linux kernel developer
> -----Original Message----- > From: sathyanarayanan kuppuswamy > [mailto:sathyanarayanan.kuppuswamy@linux.intel.com] > Sent: Wednesday, November 8, 2017 12:23 PM > To: Arnd Bergmann <arnd@arndb.de>; Pali Rohár <pali.rohar@gmail.com>; > Limonciello, Mario <Mario_Limonciello@Dell.com>; Darren Hart > <dvhart@infradead.org>; Andy Shevchenko <andy@infradead.org> > Cc: Edward O'Callaghan <quasisec@google.com>; Hans de Goede > <hdegoede@redhat.com>; platform-driver-x86@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH] dell-smbios: fix string overflow > > Hi, > > I recommend using "platform/x86: dell-smbios:" in commit header. > > On 11/08/2017 04:08 AM, Arnd Bergmann wrote: > > The new sysfs code overwrites two fixed-length character arrays > > that are each one byte shorter than they need to be, to hold > > the trailing \0: > > > > drivers/platform/x86/dell-smbios.c: In function 'build_tokens_sysfs': > > drivers/platform/x86/dell-smbios.c:494:42: error: 'sprintf' writing a terminating > nul past the end of the destination [-Werror=format-overflow=] > > sprintf(buffer_location, "%04x_location", > > drivers/platform/x86/dell-smbios.c:494:3: note: 'sprintf' output 14 bytes into a > destination of size 13 > > drivers/platform/x86/dell-smbios.c:506:36: error: 'sprintf' writing a terminating > nul past the end of the destination [-Werror=format-overflow=] > > sprintf(buffer_value, "%04x_value", > > drivers/platform/x86/dell-smbios.c:506:3: note: 'sprintf' output 11 bytes into a > destination of size 10 > Don't need to include the error log in commit message. Just explaining > the issue is good enough. > > > > This changes it to just use kasprintf(), which always gets it right. > > > > Fixes: 33b9ca1e53b4 ("platform/x86: dell-smbios: Add a sysfs interface for > SMBIOS tokens") > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > --- > > drivers/platform/x86/dell-smbios.c | 12 ++++-------- > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell- > smbios.c > > index d99edd803c19..6a60db515bda 100644 > > --- a/drivers/platform/x86/dell-smbios.c > > +++ b/drivers/platform/x86/dell-smbios.c > > @@ -463,8 +463,6 @@ static struct platform_driver platform_driver = { > > > > static int build_tokens_sysfs(struct platform_device *dev) > > { > > - char buffer_location[13]; > > - char buffer_value[10]; > > char *location_name; > > char *value_name; > > size_t size; > > @@ -491,9 +489,8 @@ static int build_tokens_sysfs(struct platform_device > *dev) > > if (da_tokens[i].tokenID == 0) > > continue; > > /* add location */ > > - sprintf(buffer_location, "%04x_location", > > - da_tokens[i].tokenID); > > - location_name = kstrdup(buffer_location, GFP_KERNEL); > > + location_name = kasprintf(GFP_KERNEL, "%04x_location", > > + da_tokens[i].tokenID); > > if (location_name == NULL) > > goto out_unwind_strings; > > sysfs_attr_init(&token_location_attrs[i].attr); > > @@ -503,9 +500,8 @@ static int build_tokens_sysfs(struct platform_device > *dev) > > token_attrs[j++] = &token_location_attrs[i].attr; > > > > /* add value */ > > - sprintf(buffer_value, "%04x_value", > > - da_tokens[i].tokenID); > > - value_name = kstrdup(buffer_value, GFP_KERNEL); > > + value_name = kasprintf(GFP_KERNEL, "%04x_value", > > + da_tokens[i].tokenID); > > if (value_name == NULL) > > goto loop_fail_create_value; > > sysfs_attr_init(&token_value_attrs[i].attr); > > -- Arnd, Assuming Darren will do the fixup for title and message as recommended by Sathyanarayanan before committing: Acked-by: Mario Limonciello <mario.limonciello@dell.com> Thanks, for the fix. For my own information and improvement, would you share how you caught that? Just added "-Werror=format-overflow=" to CFLAGS? I wasn't seeing the compile errors with default flags in my own testing, so I'd like to make sure I'm doing better testing in the future.
On Wed, Nov 8, 2017 at 8:30 PM, <Mario.Limonciello@dell.com> wrote: >> I recommend using "platform/x86: dell-smbios:" in commit header. While we (maintainers) are fixing this manually, it would be better if contributors will do this themselves :-) >> On 11/08/2017 04:08 AM, Arnd Bergmann wrote: >> > The new sysfs code overwrites two fixed-length character arrays >> > that are each one byte shorter than they need to be, to hold >> > the trailing \0: >> > >> > drivers/platform/x86/dell-smbios.c: In function 'build_tokens_sysfs': >> > drivers/platform/x86/dell-smbios.c:494:42: error: 'sprintf' writing a terminating >> nul past the end of the destination [-Werror=format-overflow=] >> > sprintf(buffer_location, "%04x_location", >> > drivers/platform/x86/dell-smbios.c:494:3: note: 'sprintf' output 14 bytes into a >> destination of size 13 >> > drivers/platform/x86/dell-smbios.c:506:36: error: 'sprintf' writing a terminating >> nul past the end of the destination [-Werror=format-overflow=] >> > sprintf(buffer_value, "%04x_value", >> > drivers/platform/x86/dell-smbios.c:506:3: note: 'sprintf' output 11 bytes into a >> destination of size 10 >> Don't need to include the error log in commit message. Just explaining >> the issue is good enough. >> > >> > This changes it to just use kasprintf(), which always gets it right. Good catch followed by a fix! > Assuming Darren will do the fixup for title and message as recommended by > Sathyanarayanan before committing: > Acked-by: Mario Limonciello <mario.limonciello@dell.com> > Thanks, for the fix. For my own information and improvement, would you > share how you caught that? Just added "-Werror=format-overflow=" to CFLAGS? > I wasn't seeing the compile errors with default flags in my own testing, so I'd like > to make sure I'm doing better testing in the future. gcc7 by its default [1], though you need to revert [2]. And always good to remember % make W=1 which implies some warning, W=2 a lot more. [1]: http://patches.linaro.org/cover/107779/ [2]: commit bd664f6b3e376a8ef4990f87d08271cc2d01ba9a Author: Linus Torvalds <torvalds@linux-foundation.org> Date: Wed Jul 12 19:25:47 2017 -0700 disable new gcc-7.1.1 warnings for now -- With Best Regards, Andy Shevchenko
On Wed, Nov 8, 2017 at 7:22 PM, sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > Hi, > > I recommend using "platform/x86: dell-smbios:" in commit header. Ok, noted. I usually try to follow the lines for each maintainer, but I'm not always keeping track of each one, sorry. > On 11/08/2017 04:08 AM, Arnd Bergmann wrote: >> >> The new sysfs code overwrites two fixed-length character arrays >> that are each one byte shorter than they need to be, to hold >> the trailing \0: >> >> drivers/platform/x86/dell-smbios.c: In function 'build_tokens_sysfs': >> drivers/platform/x86/dell-smbios.c:494:42: error: 'sprintf' writing a >> terminating nul past the end of the destination [-Werror=format-overflow=] >> sprintf(buffer_location, "%04x_location", >> drivers/platform/x86/dell-smbios.c:494:3: note: 'sprintf' output 14 bytes >> into a destination of size 13 >> drivers/platform/x86/dell-smbios.c:506:36: error: 'sprintf' writing a >> terminating nul past the end of the destination [-Werror=format-overflow=] >> sprintf(buffer_value, "%04x_value", >> drivers/platform/x86/dell-smbios.c:506:3: note: 'sprintf' output 11 bytes >> into a destination of size 10 > > Don't need to include the error log in commit message. Just explaining the > issue is good enough. I always include the messages I get, it helps a lot when you run into a related problem and find it in either the git log or using google search. It's particularly useful when patches that introduce warnings get backported to stable kernels. Arnd
On Wed, Nov 8, 2017 at 8:03 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Nov 8, 2017 at 8:30 PM, <Mario.Limonciello@dell.com> wrote: > > % make W=1 > > which implies some warning, W=2 a lot more. > > [1]: http://patches.linaro.org/cover/107779/ > > [2]: commit bd664f6b3e376a8ef4990f87d08271cc2d01ba9a > Author: Linus Torvalds <torvalds@linux-foundation.org> > Date: Wed Jul 12 19:25:47 2017 -0700 > > disable new gcc-7.1.1 warnings for now I have reverted that patch locally and this is the first time I actually hit a warning since it landed, but that reminds me that I should check whether all my workarounds have landed in mainline and we are ready to turn it on globally again. Arnd
On Wed, Nov 08, 2017 at 01:08:39PM +0100, Arnd Bergmann wrote: > The new sysfs code overwrites two fixed-length character arrays > that are each one byte shorter than they need to be, to hold > the trailing \0: > > drivers/platform/x86/dell-smbios.c: In function 'build_tokens_sysfs': > drivers/platform/x86/dell-smbios.c:494:42: error: 'sprintf' writing a terminating nul past the end of the destination [-Werror=format-overflow=] > sprintf(buffer_location, "%04x_location", > drivers/platform/x86/dell-smbios.c:494:3: note: 'sprintf' output 14 bytes into a destination of size 13 > drivers/platform/x86/dell-smbios.c:506:36: error: 'sprintf' writing a terminating nul past the end of the destination [-Werror=format-overflow=] > sprintf(buffer_value, "%04x_value", > drivers/platform/x86/dell-smbios.c:506:3: note: 'sprintf' output 11 bytes into a destination of size 10 > > This changes it to just use kasprintf(), which always gets it right. > > Fixes: 33b9ca1e53b4 ("platform/x86: dell-smbios: Add a sysfs interface for SMBIOS tokens") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Queued, thanks Arnd. Yes, please keep the error messages. Costs us nothing and can be useful to have. I corrected the prefix as noted, and added the details of the gcc and reverted patch for reproducer context. -- Darren Hart VMware Open Source Technology Center
On Wednesday 08 November 2017 13:08:39 Arnd Bergmann wrote: > The new sysfs code overwrites two fixed-length character arrays > that are each one byte shorter than they need to be, to hold > the trailing \0: > > drivers/platform/x86/dell-smbios.c: In function 'build_tokens_sysfs': > drivers/platform/x86/dell-smbios.c:494:42: error: 'sprintf' writing a terminating nul past the end of the destination [-Werror=format-overflow=] > sprintf(buffer_location, "%04x_location", > drivers/platform/x86/dell-smbios.c:494:3: note: 'sprintf' output 14 bytes into a destination of size 13 > drivers/platform/x86/dell-smbios.c:506:36: error: 'sprintf' writing a terminating nul past the end of the destination [-Werror=format-overflow=] > sprintf(buffer_value, "%04x_value", > drivers/platform/x86/dell-smbios.c:506:3: note: 'sprintf' output 11 bytes into a destination of size 10 > > This changes it to just use kasprintf(), which always gets it right. > > Fixes: 33b9ca1e53b4 ("platform/x86: dell-smbios: Add a sysfs interface for SMBIOS tokens") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Reviewed-by: Pali Rohár <pali.rohar@gmail.com> > --- > drivers/platform/x86/dell-smbios.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c > index d99edd803c19..6a60db515bda 100644 > --- a/drivers/platform/x86/dell-smbios.c > +++ b/drivers/platform/x86/dell-smbios.c > @@ -463,8 +463,6 @@ static struct platform_driver platform_driver = { > > static int build_tokens_sysfs(struct platform_device *dev) > { > - char buffer_location[13]; > - char buffer_value[10]; > char *location_name; > char *value_name; > size_t size; > @@ -491,9 +489,8 @@ static int build_tokens_sysfs(struct platform_device *dev) > if (da_tokens[i].tokenID == 0) > continue; > /* add location */ > - sprintf(buffer_location, "%04x_location", > - da_tokens[i].tokenID); > - location_name = kstrdup(buffer_location, GFP_KERNEL); > + location_name = kasprintf(GFP_KERNEL, "%04x_location", > + da_tokens[i].tokenID); > if (location_name == NULL) > goto out_unwind_strings; > sysfs_attr_init(&token_location_attrs[i].attr); > @@ -503,9 +500,8 @@ static int build_tokens_sysfs(struct platform_device *dev) > token_attrs[j++] = &token_location_attrs[i].attr; > > /* add value */ > - sprintf(buffer_value, "%04x_value", > - da_tokens[i].tokenID); > - value_name = kstrdup(buffer_value, GFP_KERNEL); > + value_name = kasprintf(GFP_KERNEL, "%04x_value", > + da_tokens[i].tokenID); > if (value_name == NULL) > goto loop_fail_create_value; > sysfs_attr_init(&token_value_attrs[i].attr); -- Pali Rohár pali.rohar@gmail.com
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c index d99edd803c19..6a60db515bda 100644 --- a/drivers/platform/x86/dell-smbios.c +++ b/drivers/platform/x86/dell-smbios.c @@ -463,8 +463,6 @@ static struct platform_driver platform_driver = { static int build_tokens_sysfs(struct platform_device *dev) { - char buffer_location[13]; - char buffer_value[10]; char *location_name; char *value_name; size_t size; @@ -491,9 +489,8 @@ static int build_tokens_sysfs(struct platform_device *dev) if (da_tokens[i].tokenID == 0) continue; /* add location */ - sprintf(buffer_location, "%04x_location", - da_tokens[i].tokenID); - location_name = kstrdup(buffer_location, GFP_KERNEL); + location_name = kasprintf(GFP_KERNEL, "%04x_location", + da_tokens[i].tokenID); if (location_name == NULL) goto out_unwind_strings; sysfs_attr_init(&token_location_attrs[i].attr); @@ -503,9 +500,8 @@ static int build_tokens_sysfs(struct platform_device *dev) token_attrs[j++] = &token_location_attrs[i].attr; /* add value */ - sprintf(buffer_value, "%04x_value", - da_tokens[i].tokenID); - value_name = kstrdup(buffer_value, GFP_KERNEL); + value_name = kasprintf(GFP_KERNEL, "%04x_value", + da_tokens[i].tokenID); if (value_name == NULL) goto loop_fail_create_value; sysfs_attr_init(&token_value_attrs[i].attr);
The new sysfs code overwrites two fixed-length character arrays that are each one byte shorter than they need to be, to hold the trailing \0: drivers/platform/x86/dell-smbios.c: In function 'build_tokens_sysfs': drivers/platform/x86/dell-smbios.c:494:42: error: 'sprintf' writing a terminating nul past the end of the destination [-Werror=format-overflow=] sprintf(buffer_location, "%04x_location", drivers/platform/x86/dell-smbios.c:494:3: note: 'sprintf' output 14 bytes into a destination of size 13 drivers/platform/x86/dell-smbios.c:506:36: error: 'sprintf' writing a terminating nul past the end of the destination [-Werror=format-overflow=] sprintf(buffer_value, "%04x_value", drivers/platform/x86/dell-smbios.c:506:3: note: 'sprintf' output 11 bytes into a destination of size 10 This changes it to just use kasprintf(), which always gets it right. Fixes: 33b9ca1e53b4 ("platform/x86: dell-smbios: Add a sysfs interface for SMBIOS tokens") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/platform/x86/dell-smbios.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) -- 2.9.0