* [PATCH] platform/x86: think-lmi: Return EINVAL when kbdlang gets set to a 0 length string
@ 2021-06-21 13:23 Hans de Goede
2021-06-21 13:58 ` Andy Shevchenko
0 siblings, 1 reply; 5+ messages in thread
From: Hans de Goede @ 2021-06-21 13:23 UTC (permalink / raw)
To: Mark Gross, Andy Shevchenko, Mark Pearson
Cc: Hans de Goede, platform-driver-x86
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(-)
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);
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] platform/x86: think-lmi: Return EINVAL when kbdlang gets set to a 0 length string
2021-06-21 13:23 [PATCH] platform/x86: think-lmi: Return EINVAL when kbdlang gets set to a 0 length string Hans de Goede
@ 2021-06-21 13:58 ` Andy Shevchenko
2021-06-21 14:13 ` Hans de Goede
0 siblings, 1 reply; 5+ messages in thread
From: Andy Shevchenko @ 2021-06-21 13:58 UTC (permalink / raw)
To: Hans de Goede; +Cc: Mark Gross, Andy Shevchenko, Mark Pearson, Platform Driver
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.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] platform/x86: think-lmi: Return EINVAL when kbdlang gets set to a 0 length string
2021-06-21 13:58 ` Andy Shevchenko
@ 2021-06-21 14:13 ` Hans de Goede
2021-06-21 16:16 ` Andy Shevchenko
0 siblings, 1 reply; 5+ messages in thread
From: Hans de Goede @ 2021-06-21 14:13 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Mark Gross, Andy Shevchenko, Mark Pearson, Platform Driver
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] platform/x86: think-lmi: Return EINVAL when kbdlang gets set to a 0 length string
2021-06-21 14:13 ` Hans de Goede
@ 2021-06-21 16:16 ` Andy Shevchenko
2021-06-21 19:29 ` Hans de Goede
0 siblings, 1 reply; 5+ messages in thread
From: Andy Shevchenko @ 2021-06-21 16:16 UTC (permalink / raw)
To: Hans de Goede; +Cc: Mark Gross, Andy Shevchenko, Mark Pearson, Platform Driver
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?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] platform/x86: think-lmi: Return EINVAL when kbdlang gets set to a 0 length string
2021-06-21 16:16 ` Andy Shevchenko
@ 2021-06-21 19:29 ` Hans de Goede
0 siblings, 0 replies; 5+ messages in thread
From: Hans de Goede @ 2021-06-21 19:29 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Mark Gross, Andy Shevchenko, Mark Pearson, Platform Driver
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-06-21 19:29 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-21 13:23 [PATCH] platform/x86: think-lmi: Return EINVAL when kbdlang gets set to a 0 length string Hans de Goede
2021-06-21 13:58 ` Andy Shevchenko
2021-06-21 14:13 ` Hans de Goede
2021-06-21 16:16 ` Andy Shevchenko
2021-06-21 19:29 ` Hans de Goede
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.