| Message ID | 20210621193648.44138-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v2] platform/x86: think-lmi: Return EINVAL when kbdlang gets set to a 0 length string | expand |
Hi, On 6/21/21 9:36 PM, Hans de Goede wrote: > Commit 0ddcf3a6b442 ("platform/x86: think-lmi: Avoid potential read before > start of the buffer") moved the length == 0 up to before stripping the '\n' > which typically gets added when users echo a value to a sysfs-attribute > from the shell. > > This avoids a potential buffer-underrun, but it also causes a behavioral > change, prior to this change "echo > kbdlang", iow writing just a single > '\n' would result in an EINVAL error, but after the change this gets > accepted setting kbdlang to an empty string. > > Fix this by replacing the manual '\n' check with using strchrnul() to get > the length till '\n' or terminating 0 in one go; and then do the > length != 0 check after this. > > Fixes: 0ddcf3a6b442 ("platform/x86: think-lmi: Avoid potential read before start of the buffer") > Reported-by: Juha Leppänen <juha_efku@dnainternet.net> > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> I've added this patch to my review-hans (soon to be for-next) branch now, Regards, Hans > --- > Changes in v2: > - Use strchrnul() to get the length and strip any trailing '\n' in one go > --- > drivers/platform/x86/think-lmi.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c > index c6c9fbb8a53e..b57061079288 100644 > --- a/drivers/platform/x86/think-lmi.c > +++ b/drivers/platform/x86/think-lmi.c > @@ -442,14 +442,9 @@ static ssize_t kbdlang_store(struct kobject *kobj, > struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj); > int length; > > - length = strlen(buf); > - if (!length) > - return -EINVAL; > - > - if (buf[length-1] == '\n') > - length--; > - > - if (length >= TLMI_LANG_MAXLEN) > + /* Calculate length till '\n' or terminating 0 */ > + length = strchrnul(buf, '\n') - buf; > + if (!length || length >= TLMI_LANG_MAXLEN) > return -EINVAL; > > memcpy(setting->kbdlang, buf, length); >
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c index c6c9fbb8a53e..b57061079288 100644 --- a/drivers/platform/x86/think-lmi.c +++ b/drivers/platform/x86/think-lmi.c @@ -442,14 +442,9 @@ static ssize_t kbdlang_store(struct kobject *kobj, struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj); int length; - length = strlen(buf); - if (!length) - return -EINVAL; - - if (buf[length-1] == '\n') - length--; - - if (length >= TLMI_LANG_MAXLEN) + /* Calculate length till '\n' or terminating 0 */ + length = strchrnul(buf, '\n') - buf; + if (!length || length >= TLMI_LANG_MAXLEN) return -EINVAL; memcpy(setting->kbdlang, buf, length);
Commit 0ddcf3a6b442 ("platform/x86: think-lmi: Avoid potential read before start of the buffer") moved the length == 0 up to before stripping the '\n' which typically gets added when users echo a value to a sysfs-attribute from the shell. This avoids a potential buffer-underrun, but it also causes a behavioral change, prior to this change "echo > kbdlang", iow writing just a single '\n' would result in an EINVAL error, but after the change this gets accepted setting kbdlang to an empty string. Fix this by replacing the manual '\n' check with using strchrnul() to get the length till '\n' or terminating 0 in one go; and then do the length != 0 check after this. Fixes: 0ddcf3a6b442 ("platform/x86: think-lmi: Avoid potential read before start of the buffer") Reported-by: Juha Leppänen <juha_efku@dnainternet.net> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2: - Use strchrnul() to get the length and strip any trailing '\n' in one go --- drivers/platform/x86/think-lmi.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)