linux-kernel - Re: [PATCH] firmware: fix async/manual firmware loading

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAB=NE6UNZLDhEFURvY_zP61WYNJU6CtpB+6FGXCvMGLvSfTd2w@mail.gmail.com>
Date:   Thu, 10 Nov 2016 11:48:27 -0800
From:   "Luis R. Rodriguez" <mcgrof@...nel.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Yves-Alexis Perez <corsac@...sac.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Yves-Alexis Perez <corsac@...ian.org>,
        Ming Lei <ming.lei@...onical.com>,
        Johannes Berg <johannes@...solutions.net>,
        Jouni Malinen <j@...fi>, Kees Cook <keescook@...omium.org>,
        Jiri Kosina <jkosina@...e.cz>, Jiri Slaby <jslaby@...e.com>,
        Tom Gundersen <teg@...m.no>, Kay Sievers <kay@...y.org>,
        Josh Boyer <jwboyer@...oraproject.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Andy Lutomirski <luto@...capital.net>,
        Harald Hoyer <harald@...hat.com>,
        Seth Forshee <seth.forshee@...onical.com>,
        Daniel Wagner <wagi@...om.org>, "4.2+" <stable@...r.kernel.org>
Subject: Re: [PATCH] firmware: fix async/manual firmware loading

On Thu, Nov 10, 2016 at 10:52 AM, Bjorn Andersson
<bjorn.andersson@...aro.org> wrote:
> On Thu 10 Nov 08:07 PST 2016, Luis R. Rodriguez wrote:
>
>> On Thu, Nov 10, 2016 at 7:55 AM, Greg Kroah-Hartman
>> <gregkh@...uxfoundation.org> wrote:
>> > On Wed, Nov 09, 2016 at 09:39:21PM +0100, Luis R. Rodriguez wrote:
>> >> On Sun, Oct 30, 2016 at 03:50:48PM +0100, Yves-Alexis Perez wrote:
>> >> > From: Yves-Alexis Perez <corsac@...ian.org>
>> >> >
>> >> > wait_for_completion_interruptible_timeout() return value is either
>> >> > -ERESTARTSYS (in case it was interrupted), 0 (in case the timeout expired)
>> >> > or the number of jiffies left until timeout. The return value is stored in
>> >> > a long, but in _request_firmware_load() it's silently casted to an int,
>> >> > which can overflow and give a negative value, indicating an error.
>> >> >
>> >> > Fix this by re-using the timeout variable and only set retval when it's
>> >> > safe.
>> >>
>> >> Please amend the commit log as I noted in the previous response, and
>> >> resend.
>> >>
>> >> > Signed-off-by: Yves-Alexis Perez <corsac@...sac.net>
>> >> > Cc: Ming Lei <ming.lei@...onical.com>
>> >> > Cc: "Luis R. Rodriguez" <mcgrof@...nel.org>
>> >> > Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>> >>
>> >> Other than the commit log you can add on you resend:
>> >>
>> >> Acked-by: Luis R. Rodriguez.
>> >>
>> >> Modulo I don't personally thing this this is sable material but I'll let
>> >> Greg decide.
>> >
>> > Does it fix a regression?
>>
>
> Yes
>
>> Not that I am aware of, but if you consider the reported the developer
>> then yes.
>>
>
> I haven't verified that this particular use case actually worked before,
> but this code works with lower timeout values (e.g. 60 in the fallback
> case), so this looks isolated.

This is true, but as I noted the broken aspect was when the timeout
was set to the max value.

> The bug was clearly introduced in v4.0 by:
>
> 68ff2a00dbf5 "firmware_loader: handle timeout via wait_for_completion_interruptible_timeout()"
>
> So please add a Fixes: and
>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@...aro.org>

This I agree with, thanks for that, and because of this then:

Acked-by: Luis R. Rodriguez <mcgrof@...nel.org>

And because of this do recommend it for stable. I would still prefer
at least a new re-submit with the respected tags and a changed commit
log describing the reason for the fix, how the cast is an issue
exactly, and how this is a regression.

>> > A reported issue with an older kernel version
>> > that people have hit?
>>
>> Definitely not.
>>
>> >  It shouldn't be hard to figure out if a patch should be in stable or not...
>>
>> Well with the only caveat now that I am suggesting we consider remove
>> this logic completely as only 2 drivers were using it explicitly
>> (second argument to request_firmware_nowait() set to false), it seems
>> they had good reasons for it but ... this has been broken for ages and
>> we seem to be happy to compartamentalize the UMH further, its unclear
>> why we would want to expand and "fix" that instead of just removing
>> crap that never worked. Thoughts?
>>
>
> Please Luis, just stop your crusade on this code. You're grasping at
> every straw of opportunity to get this code out of the kernel,

No, I'm pointing out valid issues the code has had historically and
things folks had not realized. I already knew we could not get rid of
it, but if this was *not* a regression and if this was broken always
then clearly it was something worth considering to just remove. But as
you note, its a regression. Thanks for identifying that.

> but it
> has not been broken for ages, it works just fine and it is ABI.

Agreed.

> I'm very concerned about your mission to to "compartamentalize" this
> code when you're so certain that it's "broken crap".

Well the firmware UMH fallback code is craptastic code, use at your own risk.

 Luis