[PATCH v2 0/3] padata: fix UAF issues

[PATCH v2 0/3] padata: fix UAF issues

[Chen Ridong]

From: Chen Ridong <chenridong@huawei.com>

Fix UAF issues for padata.

---
v1->v2:
 - use synchronize_rcu to fix UAF for padata_reorder.
 - add patch to avoid UAF for 'reorder_work'

Chen Ridong (3):
  padata: add pd get/put refcnt helper
  padata: fix UAF in padata_reorder
  padata: avoid UAF for reorder_work

 kernel/padata.c | 43 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] padata: add pd get/put refcnt helper

[Chen Ridong]

From: Chen Ridong <chenridong@huawei.com>

Add helpers for pd to get/put refcnt to make code consice.

Signed-off-by: Chen Ridong <chenridong@huawei.com>
---
 kernel/padata.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/kernel/padata.c b/kernel/padata.c
index cf63d9bcf482..7d79df1e3b33 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -47,6 +47,22 @@ struct padata_mt_job_state {
 static void padata_free_pd(struct parallel_data *pd);
 static void __init padata_mt_helper(struct work_struct *work);
 
+static inline void padata_get_pd(struct parallel_data *pd)
+{
+	refcount_inc(&pd->refcnt);
+}
+
+static inline void padata_put_pd_cnt(struct parallel_data *pd, int cnt)
+{
+	if (refcount_sub_and_test(cnt, &pd->refcnt))
+		padata_free_pd(pd);
+}
+
+static inline void padata_put_pd(struct parallel_data *pd)
+{
+	padata_put_pd_cnt(pd, 1);
+}
+
 static int padata_index_to_cpu(struct parallel_data *pd, int cpu_index)
 {
 	int cpu, target_cpu;
@@ -206,7 +222,7 @@ int padata_do_parallel(struct padata_shell *ps,
 	if ((pinst->flags & PADATA_RESET))
 		goto out;
 
-	refcount_inc(&pd->refcnt);
+	padata_get_pd(pd);
 	padata->pd = pd;
 	padata->cb_cpu = *cb_cpu;
 
@@ -380,8 +396,7 @@ static void padata_serial_worker(struct work_struct *serial_work)
 	}
 	local_bh_enable();
 
-	if (refcount_sub_and_test(cnt, &pd->refcnt))
-		padata_free_pd(pd);
+	padata_put_pd_cnt(pd, cnt);
 }
 
 /**
@@ -681,8 +696,7 @@ static int padata_replace(struct padata_instance *pinst)
 	synchronize_rcu();
 
 	list_for_each_entry_continue_reverse(ps, &pinst->pslist, list)
-		if (refcount_dec_and_test(&ps->opd->refcnt))
-			padata_free_pd(ps->opd);
+		padata_put_pd(ps->opd);
 
 	pinst->flags &= ~PADATA_RESET;
 
@@ -1124,8 +1138,7 @@ void padata_free_shell(struct padata_shell *ps)
 	mutex_lock(&ps->pinst->lock);
 	list_del(&ps->list);
 	pd = rcu_dereference_protected(ps->pd, 1);
-	if (refcount_dec_and_test(&pd->refcnt))
-		padata_free_pd(pd);
+	padata_put_pd(pd);
 	mutex_unlock(&ps->pinst->lock);
 
 	kfree(ps);
-- 
2.34.1

[PATCH v2 2/3] padata: fix UAF in padata_reorder

[Chen Ridong]

From: Chen Ridong <chenridong@huawei.com>

A bug was found when run ltp test:

BUG: KASAN: slab-use-after-free in padata_find_next+0x29/0x1a0
Read of size 4 at addr ffff88bbfe003524 by task kworker/u113:2/3039206

CPU: 0 PID: 3039206 Comm: kworker/u113:2 Kdump: loaded Not tainted 6.6.0+
Workqueue: pdecrypt_parallel padata_parallel_worker
Call Trace:
<TASK>
dump_stack_lvl+0x32/0x50
print_address_description.constprop.0+0x6b/0x3d0
print_report+0xdd/0x2c0
kasan_report+0xa5/0xd0
padata_find_next+0x29/0x1a0
padata_reorder+0x131/0x220
padata_parallel_worker+0x3d/0xc0
process_one_work+0x2ec/0x5a0

If 'mdelay(10)' is added before calling 'padata_find_next' in the
'padata_reorder' function, this issue could be reproduced easily with
ltp test (pcrypt_aead01).

This can be explained as bellow:

pcrypt_aead_encrypt
...
padata_do_parallel
refcount_inc(&pd->refcnt); // add refcnt
...
padata_do_serial
padata_reorder // pd
while (1) {
padata_find_next(pd, true); // using pd
queue_work_on
...
padata_serial_worker				crypto_del_alg
padata_put_pd_cnt // sub refcnt
						padata_free_shell
						padata_put_pd(ps->pd);
						// pd is freed
// loop again, but pd is freed
// call padata_find_next, UAF
}

In the padata_reorder function, when it loops in 'while', if the alg is
deleted, the refcnt may be decreased to 0 before entering
'padata_find_next', which leads to UAF.

As mentioned in [1], do_serial is supposed to be called with BHs disabled
and always happen under RCU protection, to address this issue, add
synchronize_rcu() in 'padata_free_shell' wait for all _do_serial calls
to finish.

[1] https://lore.kernel.org/all/20221028160401.cccypv4euxikusiq@parnassus.localdomain/
[2] https://lore.kernel.org/linux-kernel/jfjz5d7zwbytztackem7ibzalm5lnxldi2eofeiczqmqs2m7o6@fq426cwnjtkm/
Fixes: b128a3040935 ("padata: allocate workqueue internally")
Signed-off-by: Chen Ridong <chenridong@huawei.com>
Signed-off-by: Qu Zicheng <quzicheng@huawei.com>
---
 kernel/padata.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/padata.c b/kernel/padata.c
index 7d79df1e3b33..de2c02a81469 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -1135,6 +1135,12 @@ void padata_free_shell(struct padata_shell *ps)
 	if (!ps)
 		return;
 
+	/*
+	 * Wait for all _do_serial calls to finish to avoid touching
+	 * freed pd's and ps's.
+	 */
+	synchronize_rcu();
+
 	mutex_lock(&ps->pinst->lock);
 	list_del(&ps->list);
 	pd = rcu_dereference_protected(ps->pd, 1);
-- 
2.34.1

[PATCH v2 3/3] padata: avoid UAF for reorder_work

[Chen Ridong]

From: Chen Ridong <chenridong@huawei.com>

Although the previous patch can avoid ps and ps UAF for _do_serial, it
can not avoid potential UAF issue for reorder_work. This issue can
happen just as below:

crypto_request			crypto_request		crypto_del_alg
padata_do_serial
  ...
  padata_reorder
    // processes all remaining
    // requests then breaks
    while (1) {
      if (!padata)
        break;
      ...
    }

				padata_do_serial
				  // new request added
				  list_add
    // sees the new request
    queue_work(reorder_work)
				  padata_reorder
				    queue_work_on(squeue->work)
...

				<kworker context>
				padata_serial_worker
				// completes new request,
				// no more outstanding
				// requests

							crypto_del_alg
							  // free pd

<kworker context>
invoke_padata_reorder
  // UAF of pd

To avoid UAF for 'reorder_work', get 'pd' ref before put 'reorder_work'
into the 'serial_wq' and put 'pd' ref until the 'serial_wq' finish.

Fixes: bbefa1dd6a6d ("crypto: pcrypt - Avoid deadlock by using per-instance padata queues")
Signed-off-by: Chen Ridong <chenridong@huawei.com>
---
 kernel/padata.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/kernel/padata.c b/kernel/padata.c
index de2c02a81469..418987056340 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -352,8 +352,14 @@ static void padata_reorder(struct parallel_data *pd)
 	smp_mb();
 
 	reorder = per_cpu_ptr(pd->reorder_list, pd->cpu);
-	if (!list_empty(&reorder->list) && padata_find_next(pd, false))
+	if (!list_empty(&reorder->list) && padata_find_next(pd, false)) {
+		/*
+		 * Other context(eg. the padata_serial_worker) can finish the request.
+		 * To avoid UAF issue, add pd ref here, and put pd ref after reorder_work finish.
+		 */
+		padata_get_pd(pd);
 		queue_work(pinst->serial_wq, &pd->reorder_work);
+	}
 }
 
 static void invoke_padata_reorder(struct work_struct *work)
@@ -364,6 +370,8 @@ static void invoke_padata_reorder(struct work_struct *work)
 	pd = container_of(work, struct parallel_data, reorder_work);
 	padata_reorder(pd);
 	local_bh_enable();
+	/* Pairs with putting the reorder_work in the serial_wq */
+	padata_put_pd(pd);
 }
 
 static void padata_serial_worker(struct work_struct *serial_work)
-- 
2.34.1

Re: [PATCH v2 1/3] padata: add pd get/put refcnt helper

[Daniel Jordan]

On Fri, Jan 10, 2025 at 06:16:37AM +0000, Chen Ridong wrote:
> From: Chen Ridong <chenridong@huawei.com>
> 
> Add helpers for pd to get/put refcnt to make code consice.
> 
> Signed-off-by: Chen Ridong <chenridong@huawei.com>

Acked-by: Daniel Jordan <daniel.m.jordan@oracle.com>

Re: [PATCH v2 2/3] padata: fix UAF in padata_reorder

[Daniel Jordan]

On Fri, Jan 10, 2025 at 06:16:38AM +0000, Chen Ridong wrote:
> From: Chen Ridong <chenridong@huawei.com>
> 
> A bug was found when run ltp test:
> 
> BUG: KASAN: slab-use-after-free in padata_find_next+0x29/0x1a0
> Read of size 4 at addr ffff88bbfe003524 by task kworker/u113:2/3039206
> 
> CPU: 0 PID: 3039206 Comm: kworker/u113:2 Kdump: loaded Not tainted 6.6.0+
> Workqueue: pdecrypt_parallel padata_parallel_worker
> Call Trace:
> <TASK>
> dump_stack_lvl+0x32/0x50
> print_address_description.constprop.0+0x6b/0x3d0
> print_report+0xdd/0x2c0
> kasan_report+0xa5/0xd0
> padata_find_next+0x29/0x1a0
> padata_reorder+0x131/0x220
> padata_parallel_worker+0x3d/0xc0
> process_one_work+0x2ec/0x5a0
> 
> If 'mdelay(10)' is added before calling 'padata_find_next' in the
> 'padata_reorder' function, this issue could be reproduced easily with
> ltp test (pcrypt_aead01).
> 
> This can be explained as bellow:
> 
> pcrypt_aead_encrypt
> ...
> padata_do_parallel
> refcount_inc(&pd->refcnt); // add refcnt
> ...
> padata_do_serial
> padata_reorder // pd
> while (1) {
> padata_find_next(pd, true); // using pd
> queue_work_on
> ...
> padata_serial_worker				crypto_del_alg
> padata_put_pd_cnt // sub refcnt
> 						padata_free_shell
> 						padata_put_pd(ps->pd);
> 						// pd is freed
> // loop again, but pd is freed
> // call padata_find_next, UAF
> }
> 
> In the padata_reorder function, when it loops in 'while', if the alg is
> deleted, the refcnt may be decreased to 0 before entering
> 'padata_find_next', which leads to UAF.
> 
> As mentioned in [1], do_serial is supposed to be called with BHs disabled
> and always happen under RCU protection, to address this issue, add
> synchronize_rcu() in 'padata_free_shell' wait for all _do_serial calls
> to finish.
> 
> [1] https://lore.kernel.org/all/20221028160401.cccypv4euxikusiq@parnassus.localdomain/
> [2] https://lore.kernel.org/linux-kernel/jfjz5d7zwbytztackem7ibzalm5lnxldi2eofeiczqmqs2m7o6@fq426cwnjtkm/
> Fixes: b128a3040935 ("padata: allocate workqueue internally")
> Signed-off-by: Chen Ridong <chenridong@huawei.com>
> Signed-off-by: Qu Zicheng <quzicheng@huawei.com>

Acked-by: Daniel Jordan <daniel.m.jordan@oracle.com>

Re: [PATCH v2 3/3] padata: avoid UAF for reorder_work

[Daniel Jordan]

On Fri, Jan 10, 2025 at 06:16:39AM +0000, Chen Ridong wrote:
...
> Fixes: bbefa1dd6a6d ("crypto: pcrypt - Avoid deadlock by using per-instance padata queues")
> Signed-off-by: Chen Ridong <chenridong@huawei.com>

Series looks good, thanks for the persistence.

Acked-by: Daniel Jordan <daniel.m.jordan@oracle.com>

> diff --git a/kernel/padata.c b/kernel/padata.c
...
>  static void invoke_padata_reorder(struct work_struct *work)
> @@ -364,6 +370,8 @@ static void invoke_padata_reorder(struct work_struct *work)
>  	pd = container_of(work, struct parallel_data, reorder_work);
>  	padata_reorder(pd);
>  	local_bh_enable();
> +	/* Pairs with putting the reorder_work in the serial_wq */

s/putting/getting/

Re: [PATCH v2 3/3] padata: avoid UAF for reorder_work

[chenridong]

On 2025/1/14 1:00, Daniel Jordan wrote:
> On Fri, Jan 10, 2025 at 06:16:39AM +0000, Chen Ridong wrote:
> ...
>> Fixes: bbefa1dd6a6d ("crypto: pcrypt - Avoid deadlock by using per-instance padata queues")
>> Signed-off-by: Chen Ridong <chenridong@huawei.com>
> 
> Series looks good, thanks for the persistence.
> 

Thank you for your patience.

Best regards,
Ridong
> Acked-by: Daniel Jordan <daniel.m.jordan@oracle.com>
> 
>> diff --git a/kernel/padata.c b/kernel/padata.c
> ...
>>  static void invoke_padata_reorder(struct work_struct *work)
>> @@ -364,6 +370,8 @@ static void invoke_padata_reorder(struct work_struct *work)
>>  	pd = container_of(work, struct parallel_data, reorder_work);
>>  	padata_reorder(pd);
>>  	local_bh_enable();
>> +	/* Pairs with putting the reorder_work in the serial_wq */
> 
> s/putting/getting/
> 

Re: [PATCH v2 0/3] padata: fix UAF issues

[Herbert Xu]

On Fri, Jan 10, 2025 at 06:16:36AM +0000, Chen Ridong wrote:
> From: Chen Ridong <chenridong@huawei.com>
> 
> Fix UAF issues for padata.
> 
> ---
> v1->v2:
>  - use synchronize_rcu to fix UAF for padata_reorder.
>  - add patch to avoid UAF for 'reorder_work'
> 
> Chen Ridong (3):
>   padata: add pd get/put refcnt helper
>   padata: fix UAF in padata_reorder
>   padata: avoid UAF for reorder_work
> 
>  kernel/padata.c | 43 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 8 deletions(-)
> 
> -- 
> 2.34.1

All applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 3/3] padata: avoid UAF for reorder_work

[Herbert Xu]

On Fri, Jan 10, 2025 at 06:16:39AM +0000, Chen Ridong wrote:
> From: Chen Ridong <chenridong@huawei.com>
> 
> Although the previous patch can avoid ps and ps UAF for _do_serial, it
> can not avoid potential UAF issue for reorder_work. This issue can
> happen just as below:
> 
> crypto_request			crypto_request		crypto_del_alg
> padata_do_serial
>   ...
>   padata_reorder
>     // processes all remaining
>     // requests then breaks
>     while (1) {
>       if (!padata)
>         break;
>       ...
>     }
> 
> 				padata_do_serial
> 				  // new request added
> 				  list_add
>     // sees the new request
>     queue_work(reorder_work)
> 				  padata_reorder
> 				    queue_work_on(squeue->work)
> ...
> 
> 				<kworker context>
> 				padata_serial_worker
> 				// completes new request,
> 				// no more outstanding
> 				// requests
> 
> 							crypto_del_alg
> 							  // free pd
> 
> <kworker context>
> invoke_padata_reorder
>   // UAF of pd

Looking back this explanation is actually broken.  The call
crypto_del_alg does not free anything immediately.  It can only
start freeing things once the final tfm user goes away.  Any crypto
request of that tfm must have completed before that happens.

If not there is a serious bug in the Crypto API.

So if crypto_del_alg is leading to a freeing of the pd while there
are still outstanding users of that tfm, then this points to a bug
in the Crypto API and not padata.

Can you still reproduce this bug easily if you revert the patches
in this series? If so we should be able to track down the real bug.

To recap, every tfm holds a ref count on the underlying crypto_alg.
All crypto requests must complete before a tfm can be freed, which
then leads to a drop of the refcount on crypto_alg.

A crypto_alg can only be freed when its ref count hits zero.  Only
then will the associated pd be freed.

So what's missing in the above picture is the entity that is freeing
the tfm, thus leading to the actual freeing of the alg and pd.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 3/3] padata: avoid UAF for reorder_work

[Chen Ridong]

On 2025/5/23 11:59, Herbert Xu wrote:
> On Fri, Jan 10, 2025 at 06:16:39AM +0000, Chen Ridong wrote:
>> From: Chen Ridong <chenridong@huawei.com>
>>
>> Although the previous patch can avoid ps and ps UAF for _do_serial, it
>> can not avoid potential UAF issue for reorder_work. This issue can
>> happen just as below:
>>
>> crypto_request			crypto_request		crypto_del_alg
>> padata_do_serial
>>   ...
>>   padata_reorder
>>     // processes all remaining
>>     // requests then breaks
>>     while (1) {
>>       if (!padata)
>>         break;
>>       ...
>>     }
>>
>> 				padata_do_serial
>> 				  // new request added
>> 				  list_add
>>     // sees the new request
>>     queue_work(reorder_work)
>> 				  padata_reorder
>> 				    queue_work_on(squeue->work)
>> ...
>>
>> 				<kworker context>
>> 				padata_serial_worker
>> 				// completes new request,
>> 				// no more outstanding
>> 				// requests
>>
>> 							crypto_del_alg
>> 							  // free pd
>>
>> <kworker context>
>> invoke_padata_reorder
>>   // UAF of pd
> 
> Looking back this explanation is actually broken.  The call
> crypto_del_alg does not free anything immediately.  It can only
> start freeing things once the final tfm user goes away.  Any crypto
> request of that tfm must have completed before that happens.
> 
> If not there is a serious bug in the Crypto API.
> 
> So if crypto_del_alg is leading to a freeing of the pd while there
> are still outstanding users of that tfm, then this points to a bug
> in the Crypto API and not padata.
> 
> Can you still reproduce this bug easily if you revert the patches
> in this series? If so we should be able to track down the real bug.
> 

Unfortunately, I did not reproduce this bug, It was mentioned:
https://lore.kernel.org/all/20221019083708.27138-6-nstange@suse.de/

We through that the kworker is asynchronous, and this scenarios may happen.

Thanks,
Ridong

> To recap, every tfm holds a ref count on the underlying crypto_alg.
> All crypto requests must complete before a tfm can be freed, which
> then leads to a drop of the refcount on crypto_alg.
> 
> A crypto_alg can only be freed when its ref count hits zero.  Only
> then will the associated pd be freed.
> 
> So what's missing in the above picture is the entity that is freeing
> the tfm, thus leading to the actual freeing of the alg and pd.
> 
> Thanks,

Re: [PATCH v2 3/3] padata: avoid UAF for reorder_work

[Herbert Xu]

On Mon, May 26, 2025 at 09:15:37AM +0800, Chen Ridong wrote:
>
> Unfortunately, I did not reproduce this bug, It was mentioned:
> https://lore.kernel.org/all/20221019083708.27138-6-nstange@suse.de/
> 
> We through that the kworker is asynchronous, and this scenarios may happen.

Right.  I think the only way this can happen is through padata_replace.
That is indeed completely asynchronous and must be guarded against
using the reference count.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt