linux-kernel - Re: [PATCH v2] firmware: fix a double abort case with fw_load_sysfs

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, 3 Mar 2020 15:05:54 +0000
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     Junyong Sun <sunjy516@...il.com>
Cc:     gregkh@...uxfoundation.org, rafael@...nel.org,
        linux-kernel@...r.kernel.org, sunjunyong@...omi.com
Subject: Re: [PATCH v2] firmware: fix a double abort case with
 fw_load_sysfs_fallback

On Tue, Mar 03, 2020 at 10:36:08AM +0800, Junyong Sun wrote:
> fw_sysfs_wait_timeout may return err with -ENOENT
> at fw_load_sysfs_fallback and firmware is already
> in abort status, no need to abort again, so skip it.
> 
> This issue is caused by concurrent situation like below:
> when thread 1# wait firmware loading, thread 2# may write
> -1 to abort loading and wakeup thread 1# before it timeout.
> so wait_for_completion_killable_timeout of thread 1# would
> return remaining time which is != 0 with fw_st->status
> FW_STATUS_ABORTED.And the results would be converted into
> err -ENOENT in __fw_state_wait_common and transfered to
> fw_load_sysfs_fallback in thread 1#.
> The -ENOENT means firmware status is already at ABORTED,
> so fw_load_sysfs_fallback no need to get mutex to abort again.
> -----------------------------
> thread 1#,wait for loading
> fw_load_sysfs_fallback
>  ->fw_sysfs_wait_timeout
>     ->__fw_state_wait_common
>        ->wait_for_completion_killable_timeout
> 
> in __fw_state_wait_common,
> ...
> 93    ret = wait_for_completion_killable_timeout(&fw_st->completion, timeout);
> 94    if (ret != 0 && fw_st->status == FW_STATUS_ABORTED)
> 95       return -ENOENT;
> 96    if (!ret)
> 97	 return -ETIMEDOUT;
> 98
> 99    return ret < 0 ? ret : 0;
> -----------------------------
> thread 2#, write -1 to abort loading
> firmware_loading_store
>  ->fw_load_abort
>    ->__fw_load_abort
>      ->fw_state_aborted
>        ->__fw_state_set
>          ->complete_all
> 
> in __fw_state_set,
> ...
> 111    if (status == FW_STATUS_DONE || status == FW_STATUS_ABORTED)
> 112       complete_all(&fw_st->completion);
> -------------------------------------------
> BTW,the double abort issue would not cause kernel panic or create an issue,
> but slow down it sometimes.The change is just a minor optimization.
> 
> Signed-off-by: Junyong Sun <sunjunyong@...omi.com>

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

  Luis