linux-kernel - Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CALCETrU4__YUGk36PN=FbuEf0SBaTrxQQqm4sWs2NrZ+6WN7jA@mail.gmail.com>
Date:   Wed, 24 May 2017 21:13:38 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     "Luis R. Rodriguez" <mcgrof@...nel.org>
Cc:     Andy Lutomirski <luto@...nel.org>,
        "Michael Kerrisk (man-pages)" <mtk.manpages@...il.com>,
        Linux API <linux-api@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Daniel Wagner <wagi@...om.org>,
        David Woodhouse <dwmw2@...radead.org>,
        jewalt@...innovations.com, rafal@...ecki.pl,
        Arend Van Spriel <arend.vanspriel@...adcom.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        "Li, Yi" <yi1.li@...ux.intel.com>, atull@...nsource.altera.com,
        Moritz Fischer <moritz.fischer@...us.com>,
        Petr Mladek <pmladek@...e.com>,
        Johannes Berg <johannes.berg@...el.com>,
        Emmanuel Grumbach <emmanuel.grumbach@...el.com>,
        Luca Coelho <luciano.coelho@...el.com>,
        Kalle Valo <kvalo@...eaurora.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>,
        AKASHI Takahiro <takahiro.akashi@...aro.org>,
        David Howells <dhowells@...hat.com>,
        Peter Jones <pjones@...hat.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Alan Cox <alan@...ux.intel.com>, "Ted Ts'o" <tytso@....edu>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Martin Fuzzey <mfuzzey@...keon.com>
Subject: Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback

On Wed, May 24, 2017 at 3:38 PM, Luis R. Rodriguez <mcgrof@...nel.org> wrote:
> On Wed, May 24, 2017 at 3:00 PM, Andy Lutomirski <luto@...nel.org> wrote:
>> On Wed, May 24, 2017 at 2:40 PM, Luis R. Rodriguez <mcgrof@...nel.org> wrote:
>>> From: Martin Fuzzey <mfuzzey@...keon.com>
>>>
>>> Commit 0cb64249ca500 ("firmware_loader: abort request if wait_for_completion
>>> is interrupted") added via 4.0 added support to abort the fallback mechanism
>>> when a signal was detected and wait_for_completion_interruptible() returned
>>> -ERESTARTSYS. Although the abort was effective we were unfortunately never
>>> really propagating this error though and as such userspace could not know
>>> why the abort happened.
>>
>> Can you give a simple example of what's going on and why it matters?
>>
>> ERESTARTSYS and friends are highly magical, and I'm not convinced that
>> allowing _request_firmware_load to return -ERESTARTSYS is actually a
>> good idea.  What if there are system calls that can't handle this
>> style of restart that start being restarted as a result?
>
> This seems to be a linux-api question, so Cc'ing them and Michael.
>
> For those not familiar it is worth explaining first the user interface.
>
> This describes the fallback mechanism of the Linux firmware API if
> direct filesystem lookup fails.

...

> While we wait we can get a
> -ERESTARTSYS since swait_event_interruptible_timeout() uses
> __swait_event_interruptible_timeout() under the hood and this in turn
> ___swait_event() which can prepare_to_swait_event() which can return
> -ERESTARTSYS on signal_pending_state().

This is too much kernel detail and too little ABI detail.

User code does some syscall.  Kernel requests firmware and that
request gets interrupted.  What syscall is this?  read(2)?  open(2)?
Something else?

mutex_lock_interruptible() returns -EINTR if interrupted.  It seems
odd to be that requesting firmware would be different.

>
> The issue discovered was that Android could issue SIGCHLD and the
> waiter gets a signal but the reason for the exact reason for the
> failure is not propagated. The proposed patch propagates -ERESTARTSYS
> when that is returned on signal_pending_state() as we wait.

Maybe SIGCHLD shouldn't interrupt firmware loading?