| Message ID | 20200714081510.6070-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [5.8,regression,fix] platform/x86: thinkpad_acpi: Revert: Use strndup_user() in dispatch_proc_write() | expand |
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!
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!
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
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.
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;
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(-)