Received-SPF: pass (google.com: domain of linux-kernel+bounces-206550-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240607104610.1.I6c23f4fdb786f7200f6f1255af57b4e9621bcc66@changeid>
 <ZmM7FovMyez71TcC@slm.duckdns.org>
In-Reply-To: <ZmM7FovMyez71TcC@slm.duckdns.org>
From: Tim Van Patten <timvp@chromium.org>
Date: Fri, 7 Jun 2024 11:33:43 -0600
Message-ID: <CAMaBtwHrprN1Ev8kJPggggVcVZCxsxMQJ=NBQ2AC_Tr1jqHoCw@mail.gmail.com>
Subject: Re: [PATCH] workqueue: Prevent delayed work UAF kernel panic
To: Tejun Heo <tj@kernel.org>
Cc: LKML <linux-kernel@vger.kernel.org>, druth@chromium.org, 
	Tim Van Patten <timvp@google.com>, Lai Jiangshan <jiangshanlai@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Fri, Jun 7, 2024 at 10:53=E2=80=AFAM Tejun Heo <tj@kernel.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 t=
han
> 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 have=
n'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 =3D 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->na=
me);
+ return;
+ }
+
  /* should have been called from irqsafe timer with irq already off */
  __queue_work(dwork->cpu, dwork->wq, &dwork->work);
 }