[PATCH v1] platform/x86: thinkpad_acpi: Limit size when call strndup_user()

[PATCH v1] platform/x86: thinkpad_acpi: Limit size when call strndup_user()

[Andy Shevchenko]

During conversion to use strndup_user() the commit 35d13c7a0512
("platform/x86: thinkpad_acpi: Use strndup_user() in dispatch_proc_write()")
missed the fact that buffer coming thru procfs is not immediately NULL
terminated. We have to limit size when calling strndup_user().

Fixes: 35d13c7a0512 ("platform/x86: thinkpad_acpi: Use strndup_user() in dispatch_proc_write()")
Reported-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/platform/x86/thinkpad_acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index f571d6254e7c..f411ad814cab 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -886,7 +886,7 @@ static ssize_t dispatch_proc_write(struct file *file,
 	if (!ibm || !ibm->write)
 		return -EINVAL;
 
-	kernbuf = strndup_user(userbuf, PAGE_SIZE);
+	kernbuf = strndup_user(userbuf, min_t(long, count, PAGE_SIZE));
 	if (IS_ERR(kernbuf))
 		return PTR_ERR(kernbuf);
 
-- 
2.27.0

Re: [PATCH v1] platform/x86: thinkpad_acpi: Limit size when call strndup_user()

[Hans de Goede]

Hi,

On 7/14/20 12:42 PM, Andy Shevchenko wrote:
> During conversion to use strndup_user() the commit 35d13c7a0512
> ("platform/x86: thinkpad_acpi: Use strndup_user() in dispatch_proc_write()")
> missed the fact that buffer coming thru procfs is not immediately NULL
> terminated. We have to limit size when calling strndup_user().
> 
> Fixes: 35d13c7a0512 ("platform/x86: thinkpad_acpi: Use strndup_user() in dispatch_proc_write()")
> Reported-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>   drivers/platform/x86/thinkpad_acpi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index f571d6254e7c..f411ad814cab 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -886,7 +886,7 @@ static ssize_t dispatch_proc_write(struct file *file,
>   	if (!ibm || !ibm->write)
>   		return -EINVAL;
>   
> -	kernbuf = strndup_user(userbuf, PAGE_SIZE);
> +	kernbuf = strndup_user(userbuf, min_t(long, count, PAGE_SIZE));
>   	if (IS_ERR(kernbuf))
>   		return PTR_ERR(kernbuf);
>   
> 

This is not going to work:

char *strndup_user(const char __user *s, long n)
{
         char *p;
         long length;

         length = strnlen_user(s, n);

         if (!length)
                 return ERR_PTR(-EFAULT);

         if (length > n)
                 return ERR_PTR(-EINVAL);


And strnlen_user is:

#ifndef __strnlen_user
#define __strnlen_user(s, n) (strnlen((s), (n)) + 1)
#endif

/*
  * Unlike strnlen, strnlen_user includes the nul terminator in
  * its returned count. Callers should check for a returned value
  * greater than N as an indication the string is too long.
  */
static inline long strnlen_user(const char __user *src, long n)
{
         if (!access_ok(src, 1))
                 return 0;
         return __strnlen_user(src, n);
}

So strnlen_user returns (n + ) for a string which is n bytes
longs, so:

         length = strnlen_user(s, n);

Will set length = n + 1, and then this check triggers:

         if (length > n)
                 return ERR_PTR(-EINVAL);

Because n + 1 > n, I also build the module with your patch
and as expected I get:

[root@x1 ~]# echo -n 0 > /proc/acpi/ibm/lcdshadow
-bash: echo: write error: Invalid argument

Note you also cannot pass count+1 because then strnlen will
return count+1 if there is no terminating 0 after count bytes
and strnlen_user will return count + 1 + 1 and we still hit
the same check (and we are trying to consume one byte too much).

Can we please just go with the revert for now?

Regards,

Hans

Re: [PATCH v1] platform/x86: thinkpad_acpi: Limit size when call strndup_user()

[Andy Shevchenko]

On Tue, Jul 14, 2020 at 4:30 PM Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On 7/14/20 12:42 PM, Andy Shevchenko wrote:

...

> > +     kernbuf = strndup_user(userbuf, min_t(long, count, PAGE_SIZE));

> This is not going to work:

You are right!

> Can we please just go with the revert for now?

Yes, I have reverted it. Sorry for troubles.

-- 
With Best Regards,
Andy Shevchenko