| Message ID | 20170720160114.3261176-2-arnd@arndb.de (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Andy Shevchenko |
| Headers | show |
On Thu, Jul 20, 2017 at 7:00 PM, Arnd Bergmann <arnd@arndb.de> wrote: > gcc points out a possible format string overflow for a large value of 'zone': > > drivers/platform/x86/alienware-wmi.c: In function 'alienware_wmi_init': > drivers/platform/x86/alienware-wmi.c:461:24: error: '%02X' directive writing between 2 and 8 bytes into a region of size 6 [-Werror=format-overflow=] > sprintf(buffer, "zone%02X", i); > ^~~~ > drivers/platform/x86/alienware-wmi.c:461:19: note: directive argument in the range [0, 2147483646] > sprintf(buffer, "zone%02X", i); > ^~~~~~~~~~ > drivers/platform/x86/alienware-wmi.c:461:3: note: 'sprintf' output between 7 and 13 bytes into a destination of size 10 > > This replaces the 'int' variable with an 'u8' to make sure > it always fits, renaming the variable to 'zone' for clarity. > > Unfortunately, gcc-7.1.1 still warns about it with that change, which > seems to be unintended by the gcc developers. I have opened a bug > against gcc with a reduced test case. As a workaround, I also > change the format string to use "%02hhX", which shuts up the > warning in that version. > Thanks, pushed to testing with slight change (+ empty lines after u8 zone; where it's applicable). I'm not going to move this to fixes queue since it looks to me not critical at all. Drop me a message if you think otherwise. > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81483 > Link: https://patchwork.ozlabs.org/patch/788415/ > Suggested-by: Andy Shevchenko <andy@infradead.org> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > v2: completely rewrite the patch after discussing with Andy > --- > drivers/platform/x86/alienware-wmi.c | 38 ++++++++++++++++++------------------ > 1 file changed, 19 insertions(+), 19 deletions(-) > > diff --git a/drivers/platform/x86/alienware-wmi.c b/drivers/platform/x86/alienware-wmi.c > index 0831b428c217..c56a9ecba959 100644 > --- a/drivers/platform/x86/alienware-wmi.c > +++ b/drivers/platform/x86/alienware-wmi.c > @@ -255,12 +255,12 @@ static int parse_rgb(const char *buf, struct platform_zone *zone) > > static struct platform_zone *match_zone(struct device_attribute *attr) > { > - int i; > - for (i = 0; i < quirks->num_zones; i++) { > - if ((struct device_attribute *)zone_data[i].attr == attr) { > + u8 zone; > + for (zone = 0; zone < quirks->num_zones; zone++) { > + if ((struct device_attribute *)zone_data[zone].attr == attr) { > pr_debug("alienware-wmi: matched zone location: %d\n", > - zone_data[i].location); > - return &zone_data[i]; > + zone_data[zone].location); > + return &zone_data[zone]; > } > } > return NULL; > @@ -420,7 +420,7 @@ static DEVICE_ATTR(lighting_control_state, 0644, show_control_state, > > static int alienware_zone_init(struct platform_device *dev) > { > - int i; > + u8 zone; > char buffer[10]; > char *name; > > @@ -457,19 +457,19 @@ static int alienware_zone_init(struct platform_device *dev) > if (!zone_data) > return -ENOMEM; > > - for (i = 0; i < quirks->num_zones; i++) { > - sprintf(buffer, "zone%02X", i); > + for (zone = 0; zone < quirks->num_zones; zone++) { > + sprintf(buffer, "zone%02hhX", zone); > name = kstrdup(buffer, GFP_KERNEL); > if (name == NULL) > return 1; > - sysfs_attr_init(&zone_dev_attrs[i].attr); > - zone_dev_attrs[i].attr.name = name; > - zone_dev_attrs[i].attr.mode = 0644; > - zone_dev_attrs[i].show = zone_show; > - zone_dev_attrs[i].store = zone_set; > - zone_data[i].location = i; > - zone_attrs[i] = &zone_dev_attrs[i].attr; > - zone_data[i].attr = &zone_dev_attrs[i]; > + sysfs_attr_init(&zone_dev_attrs[zone].attr); > + zone_dev_attrs[zone].attr.name = name; > + zone_dev_attrs[zone].attr.mode = 0644; > + zone_dev_attrs[zone].show = zone_show; > + zone_dev_attrs[zone].store = zone_set; > + zone_data[zone].location = zone; > + zone_attrs[zone] = &zone_dev_attrs[zone].attr; > + zone_data[zone].attr = &zone_dev_attrs[zone]; > } > zone_attrs[quirks->num_zones] = &dev_attr_lighting_control_state.attr; > zone_attribute_group.attrs = zone_attrs; > @@ -484,9 +484,9 @@ static void alienware_zone_exit(struct platform_device *dev) > sysfs_remove_group(&dev->dev.kobj, &zone_attribute_group); > led_classdev_unregister(&global_led); > if (zone_dev_attrs) { > - int i; > - for (i = 0; i < quirks->num_zones; i++) > - kfree(zone_dev_attrs[i].attr.name); > + u8 zone; > + for (zone = 0; zone < quirks->num_zones; zone++) > + kfree(zone_dev_attrs[zone].attr.name); > } > kfree(zone_dev_attrs); > kfree(zone_data); > -- > 2.9.0 >
On Mon, Jul 24, 2017 at 11:22 AM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Jul 20, 2017 at 7:00 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> gcc points out a possible format string overflow for a large value of 'zone': >> >> drivers/platform/x86/alienware-wmi.c: In function 'alienware_wmi_init': >> drivers/platform/x86/alienware-wmi.c:461:24: error: '%02X' directive writing between 2 and 8 bytes into a region of size 6 [-Werror=format-overflow=] >> sprintf(buffer, "zone%02X", i); >> ^~~~ >> drivers/platform/x86/alienware-wmi.c:461:19: note: directive argument in the range [0, 2147483646] >> sprintf(buffer, "zone%02X", i); >> ^~~~~~~~~~ >> drivers/platform/x86/alienware-wmi.c:461:3: note: 'sprintf' output between 7 and 13 bytes into a destination of size 10 >> >> This replaces the 'int' variable with an 'u8' to make sure >> it always fits, renaming the variable to 'zone' for clarity. >> >> Unfortunately, gcc-7.1.1 still warns about it with that change, which >> seems to be unintended by the gcc developers. I have opened a bug >> against gcc with a reduced test case. As a workaround, I also >> change the format string to use "%02hhX", which shuts up the >> warning in that version. >> > > Thanks, pushed to testing with slight change (+ empty lines after u8 > zone; where it's applicable). > I'm not going to move this to fixes queue since it looks to me not > critical at all. Drop me a message if you think otherwise. Sounds good, thanks! This instance is harmless, and the warning is now globally disabled in stable kernels (and in mainline). I plan to send a patch to re-enable the warning in mainline once all the other instances are addressed. I don't think that Greg will backport that patch, but if he does, then he may need some additional 30 patches besides this one. Arnd
diff --git a/drivers/platform/x86/alienware-wmi.c b/drivers/platform/x86/alienware-wmi.c index 0831b428c217..c56a9ecba959 100644 --- a/drivers/platform/x86/alienware-wmi.c +++ b/drivers/platform/x86/alienware-wmi.c @@ -255,12 +255,12 @@ static int parse_rgb(const char *buf, struct platform_zone *zone) static struct platform_zone *match_zone(struct device_attribute *attr) { - int i; - for (i = 0; i < quirks->num_zones; i++) { - if ((struct device_attribute *)zone_data[i].attr == attr) { + u8 zone; + for (zone = 0; zone < quirks->num_zones; zone++) { + if ((struct device_attribute *)zone_data[zone].attr == attr) { pr_debug("alienware-wmi: matched zone location: %d\n", - zone_data[i].location); - return &zone_data[i]; + zone_data[zone].location); + return &zone_data[zone]; } } return NULL; @@ -420,7 +420,7 @@ static DEVICE_ATTR(lighting_control_state, 0644, show_control_state, static int alienware_zone_init(struct platform_device *dev) { - int i; + u8 zone; char buffer[10]; char *name; @@ -457,19 +457,19 @@ static int alienware_zone_init(struct platform_device *dev) if (!zone_data) return -ENOMEM; - for (i = 0; i < quirks->num_zones; i++) { - sprintf(buffer, "zone%02X", i); + for (zone = 0; zone < quirks->num_zones; zone++) { + sprintf(buffer, "zone%02hhX", zone); name = kstrdup(buffer, GFP_KERNEL); if (name == NULL) return 1; - sysfs_attr_init(&zone_dev_attrs[i].attr); - zone_dev_attrs[i].attr.name = name; - zone_dev_attrs[i].attr.mode = 0644; - zone_dev_attrs[i].show = zone_show; - zone_dev_attrs[i].store = zone_set; - zone_data[i].location = i; - zone_attrs[i] = &zone_dev_attrs[i].attr; - zone_data[i].attr = &zone_dev_attrs[i]; + sysfs_attr_init(&zone_dev_attrs[zone].attr); + zone_dev_attrs[zone].attr.name = name; + zone_dev_attrs[zone].attr.mode = 0644; + zone_dev_attrs[zone].show = zone_show; + zone_dev_attrs[zone].store = zone_set; + zone_data[zone].location = zone; + zone_attrs[zone] = &zone_dev_attrs[zone].attr; + zone_data[zone].attr = &zone_dev_attrs[zone]; } zone_attrs[quirks->num_zones] = &dev_attr_lighting_control_state.attr; zone_attribute_group.attrs = zone_attrs; @@ -484,9 +484,9 @@ static void alienware_zone_exit(struct platform_device *dev) sysfs_remove_group(&dev->dev.kobj, &zone_attribute_group); led_classdev_unregister(&global_led); if (zone_dev_attrs) { - int i; - for (i = 0; i < quirks->num_zones; i++) - kfree(zone_dev_attrs[i].attr.name); + u8 zone; + for (zone = 0; zone < quirks->num_zones; zone++) + kfree(zone_dev_attrs[zone].attr.name); } kfree(zone_dev_attrs); kfree(zone_data);
gcc points out a possible format string overflow for a large value of 'zone': drivers/platform/x86/alienware-wmi.c: In function 'alienware_wmi_init': drivers/platform/x86/alienware-wmi.c:461:24: error: '%02X' directive writing between 2 and 8 bytes into a region of size 6 [-Werror=format-overflow=] sprintf(buffer, "zone%02X", i); ^~~~ drivers/platform/x86/alienware-wmi.c:461:19: note: directive argument in the range [0, 2147483646] sprintf(buffer, "zone%02X", i); ^~~~~~~~~~ drivers/platform/x86/alienware-wmi.c:461:3: note: 'sprintf' output between 7 and 13 bytes into a destination of size 10 This replaces the 'int' variable with an 'u8' to make sure it always fits, renaming the variable to 'zone' for clarity. Unfortunately, gcc-7.1.1 still warns about it with that change, which seems to be unintended by the gcc developers. I have opened a bug against gcc with a reduced test case. As a workaround, I also change the format string to use "%02hhX", which shuts up the warning in that version. Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81483 Link: https://patchwork.ozlabs.org/patch/788415/ Suggested-by: Andy Shevchenko <andy@infradead.org> Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- v2: completely rewrite the patch after discussing with Andy --- drivers/platform/x86/alienware-wmi.c | 38 ++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 19 deletions(-)