| Message ID | 20220830205309.312864-4-ndesaulniers@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix FORTIFY=y UBSAN_LOCAL_BOUNDS=y | expand |
On Tue, Aug 30, 2022 at 01:53:09PM -0700, Nick Desaulniers wrote: > While looking into a CONFIG_FORTIFY=y related bug, I noticed that > hid_allocate calls strlen() on a local C string variable. This variable > can only have literal string values. There is no benefit to having > FORTIFY have this be a checked strlen call, because these are literal > values. By calling strlen() explicitly in the branches of a switch, the > compiler can evaluate strlen("literal value") at compile time, rather > than at runtime. > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > --- > drivers/hid/hid-input.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c > index 48c1c02c69f4..9ad3cc88c26b 100644 > --- a/drivers/hid/hid-input.c > +++ b/drivers/hid/hid-input.c > @@ -1922,12 +1922,15 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid, > switch (application) { > case HID_GD_KEYBOARD: > suffix = "Keyboard"; > + suffix_len = strlen(suffix); > break; > case HID_GD_KEYPAD: > suffix = "Keypad"; > + suffix_len = strlen(suffix); > break; > case HID_GD_MOUSE: > suffix = "Mouse"; > + suffix_len = strlen(suffix); <snip> This seems ripe for someone to come along and go "look at this cleanup patch where I move all of this duplicated code to below it in one line!" As this is a compiler bug, why not fix the compiler? Or at least put a comment in here saying why this looks so odd to prevent it from being changed in the future. thanks, greg k-h
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index 48c1c02c69f4..9ad3cc88c26b 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -1922,12 +1922,15 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid, switch (application) { case HID_GD_KEYBOARD: suffix = "Keyboard"; + suffix_len = strlen(suffix); break; case HID_GD_KEYPAD: suffix = "Keypad"; + suffix_len = strlen(suffix); break; case HID_GD_MOUSE: suffix = "Mouse"; + suffix_len = strlen(suffix); break; case HID_DG_PEN: /* @@ -1938,36 +1941,44 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid, * will have to change it and the test suite will not be happy. */ suffix = "Stylus"; + suffix_len = strlen(suffix); break; case HID_DG_STYLUS: suffix = "Pen"; + suffix_len = strlen(suffix); break; case HID_DG_TOUCHSCREEN: suffix = "Touchscreen"; + suffix_len = strlen(suffix); break; case HID_DG_TOUCHPAD: suffix = "Touchpad"; + suffix_len = strlen(suffix); break; case HID_GD_SYSTEM_CONTROL: suffix = "System Control"; + suffix_len = strlen(suffix); break; case HID_CP_CONSUMER_CONTROL: suffix = "Consumer Control"; + suffix_len = strlen(suffix); break; case HID_GD_WIRELESS_RADIO_CTLS: suffix = "Wireless Radio Control"; + suffix_len = strlen(suffix); break; case HID_GD_SYSTEM_MULTIAXIS: suffix = "System Multi Axis"; + suffix_len = strlen(suffix); break; default: + suffix_len = 0; break; } } if (suffix) { name_len = strlen(hid->name); - suffix_len = strlen(suffix); if ((name_len < suffix_len) || strcmp(hid->name + name_len - suffix_len, suffix)) { hidinput->name = kasprintf(GFP_KERNEL, "%s %s",
While looking into a CONFIG_FORTIFY=y related bug, I noticed that hid_allocate calls strlen() on a local C string variable. This variable can only have literal string values. There is no benefit to having FORTIFY have this be a checked strlen call, because these are literal values. By calling strlen() explicitly in the branches of a switch, the compiler can evaluate strlen("literal value") at compile time, rather than at runtime. Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- drivers/hid/hid-input.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)