linux-kernel - Re: [PATCHv2] firmware: Correct handling of fw_state_wait

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]

Date:   Tue, 17 Jan 2017 08:30:37 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     "Luis R. Rodriguez" <mcgrof@...nel.org>
Cc:     Chris Wilson <chris@...is-wilson.co.uk>,
        linux-kernel-dev@...khoff.com,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Daniel Wagner <daniel.wagner@...-carit.de>,
        Ming Lei <ming.lei@...onical.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        oss-drivers@...ronome.com
Subject: Re: [PATCHv2] firmware: Correct handling of fw_state_wait_timeout()
 return value

On Tue, Jan 17, 2017 at 8:21 AM, Luis R. Rodriguez <mcgrof@...nel.org> wrote:
>>>
>>>       retval = fw_state_wait_timeout(&buf->fw_st, timeout);
>>> -     if (retval < 0) {
>>> +     if (retval == -ETIMEDOUT || retval == -ERESTARTSYS) {
>>>               mutex_lock(&fw_lock);
>>>               fw_load_abort(fw_priv);
>>>               mutex_unlock(&fw_lock);
>>
>> This is a bit messy, two other similar issues were reported before
>> and upon review I suggested Patrick Bruenn's fix with a better commit
>> log seems best fit. Patrick sent a patch Jan 4, 2017 but never followed up
>> despite my feedback on a small change on the commit log message [0]. Can you
>> try that and if that fixes it can you adjust the commit log accordingly? Please
>> note the preferred solution would be:
>>
>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> index b9ac348e8d33..c530f8b4af01 100644
>> --- a/drivers/base/firmware_class.c
>> +++ b/drivers/base/firmware_class.c
>> @@ -542,6 +542,8 @@ static struct firmware_priv *to_firmware_priv(struct device *dev)
>>
>>  static void __fw_load_abort(struct firmware_buf *buf)
>>  {
>> +       if (!buf)
>> +               return;

Allow me to try to persuade you one last time :)  My patch makes the
code more logical and easier to follow.  The code says:
in case no wake up happened - finish the wait (otherwise the waking
thread finishes it).  Adding a NULL-check would just paper over the
issue and can cause trouble down the line.  If fw_state_wait_timeout()
returned because someone woke it up - there is no reason to abort the
wait.  The wait is already finished. The buggy commit mixed up return
codes from fw_state_wait_timeout() - mixed "nobody woke us up" with
"we couldn't find the FW", that's why we need to check for specific
error codes.