linux-kernel - Re: [PATCH] workqueue: Prevent delayed work UAF kernel panic

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]

Message-ID: <CAMaBtwHrprN1Ev8kJPggggVcVZCxsxMQJ=NBQ2AC_Tr1jqHoCw@mail.gmail.com>
Date: Fri, 7 Jun 2024 11:33:43 -0600
From: Tim Van Patten <timvp@...omium.org>
To: Tejun Heo <tj@...nel.org>
Cc: LKML <linux-kernel@...r.kernel.org>, druth@...omium.org, 
	Tim Van Patten <timvp@...gle.com>, Lai Jiangshan <jiangshanlai@...il.com>
Subject: Re: [PATCH] workqueue: Prevent delayed work UAF kernel panic

Hi,

On Fri, Jun 7, 2024 at 10:53 AM Tejun Heo <tj@...nel.org> wrote:
 > To prevent kernel panics, check if the pwq and pwq->pool pointers are
> > valid before derefencing them, and discard the work if they're not.
>
> Nothing guarantees that they'd stay NULL after wq destruction, right?

It doesn't appear it's possible to re-use a wq once it's been
destroyed, so I don't
think they can be re-initialized once they're NULL (nor the __WQ_DESTROYING
flag cleared). I could certainly be wrong here though.

> > Discarding all work once __WQ_DESTROYING has been set (including from
> > the same workqueue) causes breakage, so we must check the pointers
> > directly.
>
> There's only so much protection we can offer for buggy code and I'd much
> prefer an approach where the overhead is in the destruction path rather than
> the queueing path.

That's a good point about avoiding the overhead I hadn't given enough
consideration.

A fix in the destruction path would presumably require draining the work in
drain_workqueue()  or discarding it in destroy_workqueue(). My naive
interpretation of
things would be to discard it, so the work isn't executed
preemptively, but I don't know
what the expectations are for delayed work regarding which is better:
do it early or don't
do it at all. As you've pointed out, since this is buggy code to begin
with, I don't think
there's any contract that needs to be adhered to super-closely, which
is why I'm leaning
towards the discard path.

Regardless, it doesn't appear we have a list of those delayed work
items available today.
Adding one should be possible, but that would include removing the work in
delayed_work_timer_fn() in the normal path, which would also add
overhead (and likely
more than the pointer checks, due to searching the list, etc.).

I think a better approach here would be to update
delayed_work_timer_fn() to check
__WQ_DESTROYING and discard all attempts to queue to a destroyed wq. I haven't
given this as much testing (I just kicked off a round of testing), but
it should help
reduce the overhead impact.

I'm far from an expert here, so any input is appreciated. Any thoughts
on this approach
instead?

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
--- a/kernel/workqueue.c (revision 4bd3ef41540b950cf63179be1967aac6d0937766)
+++ b/kernel/workqueue.c (date 1717780157589)
@@ -1925,6 +1925,17 @@
 {
  struct delayed_work *dwork = from_timer(dwork, t, timer);

+ /*
+ * Prevent a kernel panic by discarding work queued to a destroyed wq.
+ * This must be checked while the rcu_read_lock() is
+ * held, so destroy_workqueue() cannot nullify wq->cpu_pwq while it's
+ * being accessed here.
+ */
+ if (WARN_ON_ONCE(dwork->wq->flags & __WQ_DESTROYING)) {
+ pr_warn("workqueue %s: discarding work for destroyed wq\n", dwork->wq->name);
+ return;
+ }
+
  /* should have been called from irqsafe timer with irq already off */
  __queue_work(dwork->cpu, dwork->wq, &dwork->work);
 }