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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
---
  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





[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux