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@xxxxxxxxxxxxxxx>
> Suggested-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
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);
>
