linux-kernel - Re: [PATCH v2] workqueue: fix UAF in pwq_unbound_release

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]

Message-ID: <YO28xd4mlxgN6xY8@slm.duckdns.org>
Date:   Tue, 13 Jul 2021 06:18:13 -1000
From:   Tejun Heo <tj@...nel.org>
To:     Lai Jiangshan <jiangshanlai@...il.com>
Cc:     Yang Yingliang <yangyingliang@...wei.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Xu Qiang <xuqiang36@...wei.com>,
        Pavel Skripkin <paskripkin@...il.com>
Subject: Re: [PATCH v2] workqueue: fix UAF in pwq_unbound_release_workfn()

Hello, Lai.

On Tue, Jul 13, 2021 at 01:56:12PM +0800, Lai Jiangshan wrote:
> > Does something like the following work?
> 
> It works since it has a flush_scheduled_work() in
> alloc_and_link_pwqs(). But I don't think it works for
> workqueue_apply_unbound_cpumask() when apply_wqattrs_commit()
> is not called.

Yeah, but in that path, wq is fully initialized and will always have
existing pwqs, so the wq free path shouldn't get activated. During wq
allocation, the problem is that we're installing the first set of pwqs, so
if they fail, the workqueue doesn't have any pwqs and thus triggers
self-destruction.

> If we want to reuse the current apply_wqattrs_cleanup(), I would prefer
> something like this: (untested)
> 
> @@ -3680,15 +3676,21 @@ static void pwq_unbound_release_workfn(struct
> work_struct *work)
>                                                   unbound_release_work);
>         struct workqueue_struct *wq = pwq->wq;
>         struct worker_pool *pool = pwq->pool;
> -       bool is_last;
> +       bool is_last = false;
> 
> -       if (WARN_ON_ONCE(!(wq->flags & WQ_UNBOUND)))
> -               return;
> +       /*
> +        * when @pwq is not linked, it doesn't hold any reference to the
> +        * @wq, and @wq is invalid to access.
> +        */
> +       if (!list_empty(&pwq->pwqs_node)) {
> +               if (WARN_ON_ONCE(!(wq->flags & WQ_UNBOUND)))
> +                       return;
> 
> -       mutex_lock(&wq->mutex);
> -       list_del_rcu(&pwq->pwqs_node);
> -       is_last = list_empty(&wq->pwqs);
> -       mutex_unlock(&wq->mutex);
> +               mutex_lock(&wq->mutex);
> +               list_del_rcu(&pwq->pwqs_node);
> +               is_last = list_empty(&wq->pwqs);
> +               mutex_unlock(&wq->mutex);
> +       }
> 
>         mutex_lock(&wq_pool_mutex);
>         put_unbound_pool(pool);

But, oh yeah, this is way better.

Thanks.

-- 
tejun