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

[Hans de Goede <hdegoede@redhat.com>]

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