Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Nicolai Stange <nstange@suse.de>
To:     Steffen Klassert <steffen.klassert@secunet.com>,
        Daniel Jordan <daniel.m.jordan@oracle.com>
Cc:     Herbert Xu <herbert@gondor.apana.org.au>,
        Martin Doucha <mdoucha@suse.cz>, linux-crypto@vger.kernel.org,
        linux-kernel@vger.kernel.org, Nicolai Stange <nstange@suse.de>
Subject: [PATCH 2/5] padata: make padata_free_shell() to respect pd's ->refcnt
Date:   Wed, 19 Oct 2022 10:37:05 +0200
Message-Id: <20221019083708.27138-3-nstange@suse.de>
In-Reply-To: <20221019083708.27138-1-nstange@suse.de>
References: <20221019083708.27138-1-nstange@suse.de>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On a PREEMPT kernel, the following has been observed while running
pcrypt_aead01 from LTP:

  [ ] general protection fault: 0000 [#1] PREEMPT_RT SMP PTI
  <...>
  [ ] Workqueue: pdecrypt_parallel padata_parallel_worker
  [ ] RIP: 0010:padata_reorder+0x19/0x120
  <...>
  [ ] Call Trace:
  [ ]  padata_parallel_worker+0xa3/0xf0
  [ ]  process_one_work+0x1db/0x4a0
  [ ]  worker_thread+0x2d/0x3c0
  [ ]  ? process_one_work+0x4a0/0x4a0
  [ ]  kthread+0x159/0x180
  [ ]  ? kthread_park+0xb0/0xb0
  [ ]  ret_from_fork+0x35/0x40

The pcrypt_aead01 testcase basically runs a NEWALG/DELALG sequence for some
fixed pcrypt instance in a loop, back to back.

The problem is that once the last ->serial() in padata_serial_worker() is
getting invoked, the pcrypt requests from the selftests would signal
completion, and pcrypt_aead01 can move on and subsequently issue a DELALG.
Upon pcrypt instance deregistration, the associated padata_shell would get
destroyed, which in turn would unconditionally free the associated
parallel_data instance.

If padata_serial_worker() now resumes operation after e.g. having
previously been preempted upon the return from the last of those ->serial()
callbacks, its subsequent accesses to pd for managing the ->refcnt will
all be UAFs. In particular, if the memory backing pd has meanwhile been
reused for some new parallel_data allocation, e.g in the course of
processing another subsequent NEWALG request, the padata_serial_worker()
might find the initial ->refcnt of one and free pd from under that NEWALG
or the associated selftests respectively, leading to "secondary" UAFs such
as in the Oops above.

Note that as it currently stands, a padata_shell owns a reference on its
associated parallel_data already. So fix the UAF in padata_serial_worker()
by making padata_free_shell() to not unconditionally free the shell's
associated parallel_data, but to properly drop that reference via
padata_put_pd() instead.

Fixes: 07928d9bfc81 ("padata: Remove broken queue flushing")
Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 kernel/padata.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/padata.c b/kernel/padata.c
index 3bd1e23f089b..0bf8c80dad5a 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -1112,7 +1112,7 @@ void padata_free_shell(struct padata_shell *ps)
 
 	mutex_lock(&ps->pinst->lock);
 	list_del(&ps->list);
-	padata_free_pd(rcu_dereference_protected(ps->pd, 1));
+	padata_put_pd(rcu_dereference_protected(ps->pd, 1));
 	mutex_unlock(&ps->pinst->lock);
 
 	kfree(ps);
-- 
2.37.3