| Message ID | 20210621132354.57127-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | platform/x86: think-lmi: Return EINVAL when kbdlang gets set to a 0 length string | expand |
On Mon, Jun 21, 2021 at 4:24 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Commit 0ddcf3a6b442 ("platform/x86: think-lmi: Avoid potential read before > start of the buffer") moved the lengt == 0 up to before stripping the '\n' length > 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. And why is it a problem? I mean since we haven't yet released this in any of the kernels, the ABIU can be adjusted one way or another. > Re-add the length != 0 check after stripping the '\n' to reject this > again, as before the change.
Hi, On 6/21/21 3:58 PM, Andy Shevchenko wrote: > On Mon, Jun 21, 2021 at 4:24 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Commit 0ddcf3a6b442 ("platform/x86: think-lmi: Avoid potential read before >> start of the buffer") moved the lengt == 0 up to before stripping the '\n' > > length Ack, will fix. >> 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. > > And why is it a problem? Because there are only a couple of valid string like "de", "fr", "es" and "us". We don't know the exact set of valid strings for a certain BIOS, but we do not for sure that an empty string is not valid. Regards, Hans
On Mon, Jun 21, 2021 at 5:13 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 6/21/21 3:58 PM, Andy Shevchenko wrote: > > On Mon, Jun 21, 2021 at 4:24 PM Hans de Goede <hdegoede@redhat.com> wrote: > >> > >> Commit 0ddcf3a6b442 ("platform/x86: think-lmi: Avoid potential read before > >> start of the buffer") moved the lengt == 0 up to before stripping the '\n' > > > > length > > Ack, will fix. > > >> 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. > > > > And why is it a problem? > > Because there are only a couple of valid string like "de", "fr", "es" > and "us". We don't know the exact set of valid strings for a certain > BIOS, but we do not for sure that an empty string is not valid. Since we call strlen() on the buf it means we are expecting NUL-terminated string no matter what. In this case the p = skip_spaces(buf); length = strchrnul(buf, '\n') - p; if (!length || length >= ...) return ... seems the best, no?
Hi, On 6/21/21 6:16 PM, Andy Shevchenko wrote: > On Mon, Jun 21, 2021 at 5:13 PM Hans de Goede <hdegoede@redhat.com> wrote: >> On 6/21/21 3:58 PM, Andy Shevchenko wrote: >>> On Mon, Jun 21, 2021 at 4:24 PM Hans de Goede <hdegoede@redhat.com> wrote: >>>> >>>> Commit 0ddcf3a6b442 ("platform/x86: think-lmi: Avoid potential read before >>>> start of the buffer") moved the lengt == 0 up to before stripping the '\n' >>> >>> length >> >> Ack, will fix. >> >>>> 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. >>> >>> And why is it a problem? >> >> Because there are only a couple of valid string like "de", "fr", "es" >> and "us". We don't know the exact set of valid strings for a certain >> BIOS, but we do not for sure that an empty string is not valid. > > Since we call strlen() on the buf it means we are expecting > NUL-terminated string no matter what. > In this case the > > p = skip_spaces(buf); > length = strchrnul(buf, '\n') - p; > if (!length || length >= ...) > return ... > > seems the best, no? I don't see the need to skip any leading whitespace, we don't do that for any of the other attributes either, so that would be inconsistent, other then that I think using strchrnul to calc the length + skip the '\n' in one go is a good idea. let me send out a v2. Regards, Hans
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c index c6c9fbb8a53e..c22435acebbe 100644 --- a/drivers/platform/x86/think-lmi.c +++ b/drivers/platform/x86/think-lmi.c @@ -449,7 +449,7 @@ static ssize_t kbdlang_store(struct kobject *kobj, if (buf[length-1] == '\n') length--; - if (length >= TLMI_LANG_MAXLEN) + 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 lengt == 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. Re-add the length != 0 check after stripping the '\n' to reject this again, as before the change. Fixes: 0ddcf3a6b442 ("platform/x86: think-lmi: Avoid potential read before start of the buffer") Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/think-lmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)