linux-kernel - Re: [PATCH 5.8 regression fix] platform/x86: thinkpad_acpi: Revert: Use strndup_user() in dispatch_proc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 14 Jul 2020 13:43:01 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     Darren Hart <dvhart@...radead.org>,
        Andy Shevchenko <andy@...radead.org>,
        Henrique de Moraes Holschuh <ibm-acpi@....eng.br>,
        Thinkpad-acpi devel ML <ibm-acpi-devel@...ts.sourceforge.net>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 5.8 regression fix] platform/x86: thinkpad_acpi: Revert:
 Use strndup_user() in dispatch_proc_write()

On Tue, Jul 14, 2020 at 12:33 PM Hans de Goede <hdegoede@...hat.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@...il.com> wrote:
> >> On Tue, Jul 14, 2020 at 11:15 AM Hans de Goede <hdegoede@...hat.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