linux-kernel - Re: [PATCH v4 2/4] firmware: encapsulate firmware loading status

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:   Fri, 9 Sep 2016 13:43:45 +0200
From:   Daniel Wagner <daniel.wagner@...-carit.de>
To:     Ming Lei <ming.lei@...onical.com>, Daniel Wagner <wagi@...om.org>
CC:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "Luis R . Rodriguez" <mcgrof@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v4 2/4] firmware: encapsulate firmware loading status

On 09/08/2016 05:49 PM, Ming Lei wrote:
> On Thu, Sep 8, 2016 at 8:26 PM, Daniel Wagner <wagi@...om.org> wrote:
>>
>>         if (ret != 0 && test_bit(...))
>>                 return -ENOENT;
>
> Another question, why do you want to return -ENOENT when
> userspace aborts the load? And looks it will always be override as
> -EAGAIN.

I took the -ENOENT from sync_cached_firmware_buf():

  /* wait until the shared firmware_buf becomes ready (or error) */
  static int sync_cached_firmware_buf(struct firmware_buf *buf)
  {
  	int ret = 0;

  	mutex_lock(&fw_lock);
-	while (!test_bit(FW_STATUS_DONE, &buf->status)) {
-		if (is_fw_load_aborted(buf)) {
+	while (!fw_status_is_done(&buf->fw_st)) {
+		if (fw_status_is_aborted(&buf->fw_st)) {
  			ret = -ENOENT;
  			break;
  		}
  		mutex_unlock(&fw_lock);
-		ret = wait_for_completion_interruptible(&buf->completion);
+		ret = fw_status_wait_timeout(&buf->fw_st, 0);
  		mutex_lock(&fw_lock);


There is a path where it not overwritten by -EAGAIN:

   request_firmware()
     _request_firmware()
       _request_firmware_prepare
         sync_cached_firmware_buf()
   	  fw_status_wait_timeout()
	    return -ENOENT

If you want me to change it to EAGAIN, I'll do it. I just tried hard not 
to have any changes in behavior.

cheer,
daniel