| Message ID | 20200714104250.87970-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | [v1] platform/x86: thinkpad_acpi: Limit size when call strndup_user() | expand |
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@redhat.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > 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
On Tue, Jul 14, 2020 at 4:30 PM Hans de Goede <hdegoede@redhat.com> 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.
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);
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@redhat.com> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/platform/x86/thinkpad_acpi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)