linux-kernel - Re: [PATCH] firmware loader: Fix the concurrent request_firmware() race for kref

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:	Fri, 9 Nov 2012 16:16:00 +0800
From:	Ming Lei <ming.lei@...onical.com>
To:	Chuansheng Liu <chuansheng.liu@...el.com>
Cc:	gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] firmware loader: Fix the concurrent request_firmware()
 race for kref_get/put

On Fri, Nov 9, 2012 at 8:28 PM, Chuansheng Liu <chuansheng.liu@...el.com> wrote:
>
> There is one race that both request_firmware() with the same
> firmware name.
>
> The race scenerio is as below:
> CPU1                                                  CPU2
> request_firmware() -->
> _request_firmware_load() return err                   another request_firmware() is coming -->
> _request_firmware_cleanup is called -->               _request_firmware_prepare -->
> release_firmware --->                                 fw_lookup_and_allocate_buf -->
>                                                       spin_lock(&fwc->lock)
> ...                                                   __fw_lookup_buf() return true
> fw_free_buf() will be called -->                      ...
> kref_put -->
> decrease the refcount to 0
>                                                       kref_get(&tmp->ref) ==> it will trigger warning
>                                                                               due to refcount == 0
> __fw_free_buf() -->
> ...                                                   spin_unlock(&fwc->lock)
> spin_lock(&fwc->lock)
> list_del(&buf->list)
> spin_unlock(&fwc->lock)
> kfree(buf)
>                                                       After that, the freed buf will be used.
>
> The key race is decreasing refcount to 0 and list_del is not protected together by
> fwc->lock, and it is possible another thread try to get it between refcount==0
> and list_del.
>
> Fix it here to protect it together.

Good catch, the reference count and the list operation should be atomic.

>
> Signed-off-by: liu chuansheng <chuansheng.liu@...el.com>
> ---
>  drivers/base/firmware_class.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index b44ed35..7df32cd 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -246,7 +246,6 @@ static void __fw_free_buf(struct kref *ref)
>                  __func__, buf->fw_id, buf, buf->data,
>                  (unsigned int)buf->size);
>
> -       spin_lock(&fwc->lock);
>         list_del(&buf->list);
>         spin_unlock(&fwc->lock);
>
> @@ -263,7 +262,10 @@ static void __fw_free_buf(struct kref *ref)
>
>  static void fw_free_buf(struct firmware_buf *buf)
>  {
> -       kref_put(&buf->ref, __fw_free_buf);
> +       struct firmware_cache *fwc = buf->fwc;
> +       spin_lock(&fwc->lock);
> +       if(!kref_put(&buf->ref, __fw_free_buf))

$./scripts/checkpatch.pl
ERROR: space required before the open parenthesis '('
#97: FILE: drivers/base/firmware_class.c:267:
+	if(!kref_put(&buf->ref, __fw_free_buf))


> +               spin_unlock(&fwc->lock);
>  }
>
>  /* direct firmware loading support */

After fixing the patch style error, you can add my ack:

Acked-by: Ming Lei <ming.lei@...onical.com>


Thanks,
---
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/