Re: [PATCH v2] workqueue: fix UAF in pwq_unbound_release_workfn()

[Tejun Heo <tj@kernel.org>]

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