[PATCH] platform/x86: think-lmi: Return EINVAL when kbdlang gets set to a 0 length string

[PATCH] platform/x86: think-lmi: Return EINVAL when kbdlang gets set to a 0 length string

[Hans de Goede]

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

Re: [PATCH] platform/x86: think-lmi: Return EINVAL when kbdlang gets set to a 0 length string

[Andy Shevchenko]

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

Re: [PATCH] platform/x86: think-lmi: Return EINVAL when kbdlang gets set to a 0 length string

[Hans de Goede]

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

Re: [PATCH] platform/x86: think-lmi: Return EINVAL when kbdlang gets set to a 0 length string

[Andy Shevchenko]

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

Re: [PATCH] platform/x86: think-lmi: Return EINVAL when kbdlang gets set to a 0 length string

[Hans de Goede]

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