| 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
| ||
|
Message-ID: <20170524205658.GK8951@wotan.suse.de>
Date: Wed, 24 May 2017 22:56:58 +0200
From: "Luis R. Rodriguez" <mcgrof@...nel.org>
To: "Luis R. Rodriguez" <mcgrof@...nel.org>, jewalt@...innovations.com
Cc: Martin Fuzzey <mfuzzey@...keon.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Shuah Khan <shuah.kh@...sung.com>, wagi@...om.org,
yi1.li@...ux.intel.com, takahiro.akashi@...aro.org,
bjorn.andersson@...aro.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] firmware: request_firmware() should propagate
-ERESTARTSYS
On Tue, May 23, 2017 at 09:55:33PM +0200, Luis R. Rodriguez wrote:
> On Tue, May 23, 2017 at 04:32:49PM +0200, Martin Fuzzey wrote:
> > On 23/05/17 15:31, Greg Kroah-Hartman wrote:
> > > On Tue, May 23, 2017 at 03:16:07PM +0200, Martin Fuzzey wrote:
> > > > When -ERESTARTSYS is returned by wait_* due to a signal this should
> > > > be returned from request_firmware() so that the syscall may be
> > > > restarted if necessary.
> > > >
> > > >
> > > > Nice find, should this go to the stable kernels as well?
> >
> > Yes I think it should.
>
> Thanks for the patch !
>
> Just a bit of more nose diving validating this.
>
> We actually used to send -ENOMEM for a long time always, and now you are
> special-casing to allow -ERESTARTSYS -- we we must ask ourselves -- why
> not other errors ?
>
> Let us consider what is upstream only please and focus on stable later.
>
> We use:
>
> static int __fw_state_wait_common(struct fw_state *fw_st, long timeout)
> {
> long ret;
>
> ret = swait_event_interruptible_timeout(fw_st->wq,
> __fw_state_is_done(READ_ONCE(fw_st->status)),
> timeout);
> if (ret != 0 && fw_st->status == FW_STATUS_ABORTED)
> return -ENOENT;
> if (!ret)
> return -ETIMEDOUT;
>
> return ret < 0 ? ret : 0;
> }
>
> What can swait_event_interruptible_timeout() return ? It can return the value
> of a timeout or whatever __swait_event_interruptible_timeout() returns.
> __swait_event_interruptible_timeout() in turn uses ___swait_event() as
> follows:
>
> #define __swait_event_interruptible_timeout(wq, condition, timeout) \
> ___swait_event(wq, ___wait_cond_timeout(condition), \
> TASK_INTERRUPTIBLE, timeout, \
> __ret = schedule_timeout(__ret))
>
> So this ultimately use ___swait_event():
>
> /* as per ___wait_event() but for swait, therefore "exclusive == 0" */
> #define ___swait_event(wq, condition, state, ret, cmd) \
> ({ \
> struct swait_queue __wait; \
> long __ret = ret; \
> \
> INIT_LIST_HEAD(&__wait.task_list); \
> for (;;) { \
> long __int = prepare_to_swait_event(&wq, &__wait, state);\
> \
> if (condition) \
> break; \
> \
> if (___wait_is_interruptible(state) && __int) { \
> __ret = __int; \
> break; \
> } \
> \
> cmd; \
> } \
> finish_swait(&wq, &__wait); \
> __ret; \
> })
>
> And prepare_to_swait_event() can return -ERESTARTSYS on signal_pending_state()
> otherwise it returns 0 ! So indeed -ERESTARTSYS is possible.
>
> But what about other errors ? Considering the above it would seem then we
> actually can only get -ERESTARTSYS or whatever schedule_timeout() returns.
> schedule_timeout() is documented indicating it returns 0 when the timer has
> expired otherwise the remaining time in jiffies will be returned. It further
> clarifies that the return value is guaranteed to be be non-negative. And
> ___wait_is_interruptible() is:
>
> #define ___wait_is_interruptible(state) \
> (!__builtin_constant_p(state) || \
> state == TASK_INTERRUPTIBLE || state == TASK_KILLABLE) \
>
> So this piece of code:
> \
> if (___wait_is_interruptible(state) && __int) { \
> __ret = __int; \
> break; \
> } \
>
> Since ___wait_is_interruptible() is always true for us since we use
> __swait_event_interruptible_timeout() the -ERESTARTSYS will always be sent if a
> signal was sent.
>
> In all this light then the patch is correct for upstream however let's consider
> stable now. At first glance this seems like a fix for an old patch,
> respectively commit 0542ad88fbdd81bb ("firmware loader: Fix
> _request_firmware_load() return val for fw load abort" by Shuah Khan which was
> merged since v3.17, *but* back then just used wait_for_completion() and ignored
> any signals here, they were just not part of our semantics. But its important
> to note then we always returned -ENOMEM before that patch and then at least
> returned -EAGAIN in other cases. The next thing to note is commit
> 5d47ec02c37ea632398c ("firmware: Correct handling of fw_state_wait() return
> value") by Bjorn Andersson. This took place *after* the swait changes. Bjorn
> fixed an issue but also forgot to address the special case of -ERESTARTSYS,
> given right below fw_state_wait_timeout() on _request_firmware_load() the
> return value would be lost. It would seem Bjorn assumed the return value would
> be propagated but did not notice the error special casing below which would
> loose it.
>
> So before and after Bjorn's changes we were still *trying* to propagate the
> -ERESTARTSYS error but it was still lost.
>
> The -ERESTARTSYS from signals was still something we were capturing even prior
> to the swait changes, see the kernel commit prior to the swait changes (0430cafcc4fb
> "firmware: drop bit ops in favor of simple state machine"), if you git blame there,
> will find a series of commits with the -ERESTARTSYS handled... I can trace back
> to commit 68ff2a00dbf ("firmware_loader: handle timeout via
> wait_for_completion_interruptible_timeout()") as having the -ERESTARTSYS check but
> it had lost that error on _request_firmware_load() due to the :
>
> if (retval == -ERESTARTSYS || !retval) {
> mutex_lock(&fw_lock);
> fw_load_abort(fw_priv);
> mutex_unlock(&fw_lock);
> }
>
> if (is_fw_load_aborted(buf))
> retval = -EAGAIN;
> else if (!buf->data)
> retval = -ENOMEM;
>
> As noted earlier the above piece of code lost the error because of
> 0542ad88fbdd81bb ("firmware loader: Fix _request_firmware_load() return val for
> fw load abort" which was merged since v3.17, but back then it *did not*
> propagate the error. So it would be incorrect to say that your patch fixes
> commit 0542ad88fbdd81bb by Shuah Khan... We'd have to ask ourselves when
> such an error actually became relevant.
>
> It would seem the -ERESTARTSYS signal took effect first via commit 0cb64249ca500
> ("firmware_loader: abort request if wait_for_completion is interrupted") added
> upstream via v4.0 and since it was introduced the error code was lost given the
> -EAGAIN overwrite added *earlier* by Shuah Khan when such error codes were not
> even relevant. So prior to v4.0 were we not even aborting due to signals.
>
> As such this as far as I can tell this is a fix for a fix for this commit.
>
> So we should use:
>
> Fixes: 0cb64249ca500 ("firmware_loader: abort request if wait_for_completion is interrupted")
> Cc: stable@...r.kernel.org # 4.0
>
> Also a more reflective subject and commit log would be appreciated, you can add
> my Acked-by given I have also now tested it with all the test drivers and
> test scripts:
>
> ==========================================================================
> firmware: fix sending -ERESTARTSYS due to signal on fallback
>
> 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.
>
> The error code was always being lost to an even older change, commit
> 0542ad88fbdd81bb ("firmware loader: Fix _request_firmware_load() return val
> for fw load abort") by Shuah Khan which was merged since v3.17. Back then
> though we never were capturing these signals or bailing on a signal. After
> this change though only only -EAGAIN was being relayed back to userspace
> on non-memory errors including signals trying to interrupt our fallback
> process.
>
> It only makes sense to fix capturing -ERESTARTSYS since we were capturing
> the error but when it was actually effective, since commit 0cb64249ca500
> ("firmware_loader: abort request if wait_for_completion is interrupted").
>
> Only distributions relying on the fallback mechanism are impacted. An
> example issue is on Android, when request_firmware() is called through
> the firmware fallback mechanism -- syfs write call, sysfs .store(), and
> Android sends a SIGCHLD to fail the write call -- in such cases the fact
> that we failed due to a signal is lost.
>
> Fix this and ensure we propagate -ERESTARTSYS so that handlers can whether
> or not to restart write calls.
>
> Signed-off-by: Martin Fuzzey <mfuzzey@...keon.com>
> Cc: stable@...r.kernel.org # 4.0
> Acked-by: Luis R. Rodriguez <mcgrof@...nel.org>
> ==========================================================================
>
> > I have already applied a similar patch to my 4.4 tree.
>
> You might also be intersted in commit 2e700f8d85975 ("firmware: fix usermode
> helper fallback loading".
>
> > The exact same patch won't apply since the code has changed a bit since.
> >
> > So I was planning on sending for -stable-4.4 once it's in mainline.
>
> Appreciated, after this is merged of course.
I'll just send this with the commit log change myself.
Luis
Powered by blists - more mailing lists