Messages in this thread | | | Date | Tue, 13 Jul 2021 06:18:13 -1000 | From | Tejun Heo <> | 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
| |