[PATCH 5.8 regression fix] platform/x86: thinkpad_acpi: Revert: Use strndup_user() in dispatch_proc_write()

[PATCH 5.8 regression fix] platform/x86: thinkpad_acpi: Revert: Use strndup_user() in dispatch_proc_write()

[Hans de Goede]

Commit 35d13c7a0512 ("platform/x86: thinkpad_acpi: Use strndup_user()
in dispatch_proc_write()") cleaned up dispatch_proc_write() by replacing
the code to copy the passed in data from userspae with strndup_user().

But strndup_user() expects a 0 terminated input buffer and the buffer
passed to dispatch_proc_write() is NOT 0 terminated.

So this change leads to strndup_user() copying some extra random bytes
from userspace till it hits a 0 byte.

This commit reverts the change to use strndup_user() fixing the
buffer being passed to the ibm_struct.write() call back containing extra
junk at the end.

Fixes: 35d13c7a0512 ("platform/x86: thinkpad_acpi: Use strndup_user() in dispatch_proc_write()")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 92aad746d1f8..8ae2be5871f5 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -886,11 +886,19 @@ static ssize_t dispatch_proc_write(struct file *file,
 
 	if (!ibm || !ibm->write)
 		return -EINVAL;
+	if (count > PAGE_SIZE - 1)
+		return -EINVAL;
+
+	kernbuf = kmalloc(count + 1, GFP_KERNEL);
+	if (!kernbuf)
+		return -ENOMEM;
 
-	kernbuf = strndup_user(userbuf, PAGE_SIZE);
-	if (IS_ERR(kernbuf))
-		return PTR_ERR(kernbuf);
+	if (copy_from_user(kernbuf, userbuf, count)) {
+		kfree(kernbuf);
+		return -EFAULT;
+	}
 
+	kernbuf[count] = 0;
 	ret = ibm->write(kernbuf);
 	if (ret == 0)
 		ret = count;
-- 
2.26.2

Re: [PATCH 5.8 regression fix] platform/x86: thinkpad_acpi: Revert: Use strndup_user() in dispatch_proc_write()

[Andy Shevchenko]

On Tue, Jul 14, 2020 at 11:15 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Commit 35d13c7a0512 ("platform/x86: thinkpad_acpi: Use strndup_user()
> in dispatch_proc_write()") cleaned up dispatch_proc_write() by replacing
> the code to copy the passed in data from userspae with strndup_user().

user space

> But strndup_user() expects a 0 terminated input buffer and the buffer
> passed to dispatch_proc_write() is NOT 0 terminated.
>
> So this change leads to strndup_user() copying some extra random bytes
> from userspace till it hits a 0 byte.
>
> This commit reverts the change to use strndup_user() fixing the
> buffer being passed to the ibm_struct.write() call back containing extra
> junk at the end.

Can we simply use memdup_user()?
And thanks for catching this up!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5.8 regression fix] platform/x86: thinkpad_acpi: Revert: Use strndup_user() in dispatch_proc_write()

[Andy Shevchenko]

On Tue, Jul 14, 2020 at 11:21 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Jul 14, 2020 at 11:15 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Commit 35d13c7a0512 ("platform/x86: thinkpad_acpi: Use strndup_user()
> > in dispatch_proc_write()") cleaned up dispatch_proc_write() by replacing
> > the code to copy the passed in data from userspae with strndup_user().
>
> user space
>
> > But strndup_user() expects a 0 terminated input buffer and the buffer
> > passed to dispatch_proc_write() is NOT 0 terminated.

Second though, perhaps it's a simple wrong count parameter?
strndup_user(..., min(count, PAGE_SIZE)) or so would work?

> > So this change leads to strndup_user() copying some extra random bytes
> > from userspace till it hits a 0 byte.
> >
> > This commit reverts the change to use strndup_user() fixing the
> > buffer being passed to the ibm_struct.write() call back containing extra
> > junk at the end.
>
> Can we simply use memdup_user()?
> And thanks for catching this up!



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5.8 regression fix] platform/x86: thinkpad_acpi: Revert: Use strndup_user() in dispatch_proc_write()

[Hans de Goede]

Hi,

On 7/14/20 10:27 AM, Andy Shevchenko wrote:
> On Tue, Jul 14, 2020 at 11:21 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Tue, Jul 14, 2020 at 11:15 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Commit 35d13c7a0512 ("platform/x86: thinkpad_acpi: Use strndup_user()
>>> in dispatch_proc_write()") cleaned up dispatch_proc_write() by replacing
>>> the code to copy the passed in data from userspae with strndup_user().
>>
>> user space

Ack, do you want me to send a v2, or can you fix this up after applying.

>>> But strndup_user() expects a 0 terminated input buffer and the buffer
>>> passed to dispatch_proc_write() is NOT 0 terminated.
> 
> Second though, perhaps it's a simple wrong count parameter?
> strndup_user(..., min(count, PAGE_SIZE)) or so would work?

I honestly have not looked at a better fix and given that you've just come
up with 2 suggestions which may or may not work, combined with
where we are in the 5.8 cycle I believe it would be best to just
play it safe and go with the revert for 5.8.

If you then take a second attempt at cleaning this up for 5.9 and
send it to me, I can test it for you.

Regards,

Hans

Re: [PATCH 5.8 regression fix] platform/x86: thinkpad_acpi: Revert: Use strndup_user() in dispatch_proc_write()

[Andy Shevchenko]

On Tue, Jul 14, 2020 at 12:33 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 7/14/20 10:27 AM, Andy Shevchenko wrote:
> > On Tue, Jul 14, 2020 at 11:21 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> >> On Tue, Jul 14, 2020 at 11:15 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >>>
> >>> Commit 35d13c7a0512 ("platform/x86: thinkpad_acpi: Use strndup_user()
> >>> in dispatch_proc_write()") cleaned up dispatch_proc_write() by replacing
> >>> the code to copy the passed in data from userspae with strndup_user().
> >>
> >> user space
>
> Ack, do you want me to send a v2, or can you fix this up after applying.
>
> >>> But strndup_user() expects a 0 terminated input buffer and the buffer
> >>> passed to dispatch_proc_write() is NOT 0 terminated.
> >
> > Second though, perhaps it's a simple wrong count parameter?
> > strndup_user(..., min(count, PAGE_SIZE)) or so would work?
>
> I honestly have not looked at a better fix and given that you've just come
> up with 2 suggestions which may or may not work, combined with
> where we are in the 5.8 cycle I believe it would be best to just
> play it safe and go with the revert for 5.8.
>
> If you then take a second attempt at cleaning this up for 5.9 and
> send it to me, I can test it for you.

I guess there is no need to revert. I have looked at the documentation
and see that all of the procfs parameters are normal strings, but you
are right they are not NULL terminated per se. So, the problematic
place is the size of data we retrieve from user space.

I have sent a patch.


-- 
With Best Regards,
Andy Shevchenko