[PATCH 0/2] padata: fix UAF in padata_reorder

[PATCH 0/2] padata: fix UAF in padata_reorder

[Chen Ridong]

From: Chen Ridong <chenridong@huawei.com>


Chen Ridong (2):
  padata: add pd get/put refcnt helper
  padata: fix UAF in padata_reorder

 kernel/padata.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH 1/2] 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 | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/kernel/padata.c b/kernel/padata.c
index d51bbc76b227..5d8e18cdcb25 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -47,6 +47,23 @@ 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(struct parallel_data *pd)
+{
+	if (refcount_dec_and_test(&pd->refcnt))
+		padata_free_pd(pd);
+}
+
+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 int padata_index_to_cpu(struct parallel_data *pd, int cpu_index)
 {
 	int cpu, target_cpu;
@@ -206,7 +223,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 +397,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 +697,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 +1139,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(ps->pd);
 	mutex_unlock(&ps->pinst->lock);
 
 	kfree(ps);
-- 
2.34.1

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

[kernel test robot]

Hi Chen,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.12 next-20241125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Chen-Ridong/padata-add-pd-get-put-refcnt-helper/20241125-111043
base:   linus/master
patch link:    https://lore.kernel.org/r/20241123080509.2573987-2-chenridong%40huaweicloud.com
patch subject: [PATCH 1/2] padata: add pd get/put refcnt helper
config: x86_64-randconfig-161-20241125 (https://download.01.org/0day-ci/archive/20241125/202411252108.XGoGQSTI-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241125/202411252108.XGoGQSTI-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411252108.XGoGQSTI-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> kernel/padata.c:1134:24: warning: variable 'pd' set but not used [-Wunused-but-set-variable]
    1134 |         struct parallel_data *pd;
         |                               ^
   1 warning generated.


vim +/pd +1134 kernel/padata.c

bbefa1dd6a6d53 Herbert Xu   2019-11-26  1126  
bbefa1dd6a6d53 Herbert Xu   2019-11-26  1127  /**
bbefa1dd6a6d53 Herbert Xu   2019-11-26  1128   * padata_free_shell - free a padata shell
bbefa1dd6a6d53 Herbert Xu   2019-11-26  1129   *
bbefa1dd6a6d53 Herbert Xu   2019-11-26  1130   * @ps: padata shell to free
bbefa1dd6a6d53 Herbert Xu   2019-11-26  1131   */
bbefa1dd6a6d53 Herbert Xu   2019-11-26  1132  void padata_free_shell(struct padata_shell *ps)
bbefa1dd6a6d53 Herbert Xu   2019-11-26  1133  {
7ddc21e317b360 WangJinchao  2023-10-16 @1134  	struct parallel_data *pd;
7ddc21e317b360 WangJinchao  2023-10-16  1135  
07b24c7c08bdc2 Eric Biggers 2020-02-25  1136  	if (!ps)
07b24c7c08bdc2 Eric Biggers 2020-02-25  1137  		return;
bbefa1dd6a6d53 Herbert Xu   2019-11-26  1138  
07b24c7c08bdc2 Eric Biggers 2020-02-25  1139  	mutex_lock(&ps->pinst->lock);
bbefa1dd6a6d53 Herbert Xu   2019-11-26  1140  	list_del(&ps->list);
7ddc21e317b360 WangJinchao  2023-10-16  1141  	pd = rcu_dereference_protected(ps->pd, 1);
31df8a12c672a5 Chen Ridong  2024-11-23  1142  	padata_put_pd(ps->pd);
07b24c7c08bdc2 Eric Biggers 2020-02-25  1143  	mutex_unlock(&ps->pinst->lock);
bbefa1dd6a6d53 Herbert Xu   2019-11-26  1144  
bbefa1dd6a6d53 Herbert Xu   2019-11-26  1145  	kfree(ps);
bbefa1dd6a6d53 Herbert Xu   2019-11-26  1146  }
bbefa1dd6a6d53 Herbert Xu   2019-11-26  1147  EXPORT_SYMBOL(padata_free_shell);
bbefa1dd6a6d53 Herbert Xu   2019-11-26  1148  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Chen,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.12 next-20241126]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Chen-Ridong/padata-add-pd-get-put-refcnt-helper/20241125-111043
base:   linus/master
patch link:    https://lore.kernel.org/r/20241123080509.2573987-2-chenridong%40huaweicloud.com
patch subject: [PATCH 1/2] padata: add pd get/put refcnt helper
config: x86_64-randconfig-122-20241125 (https://download.01.org/0day-ci/archive/20241126/202411261818.iINyAe83-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241126/202411261818.iINyAe83-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411261818.iINyAe83-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> kernel/padata.c:1142:25: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct parallel_data *pd @@     got struct parallel_data [noderef] __rcu *pd @@
   kernel/padata.c:1142:25: sparse:     expected struct parallel_data *pd
   kernel/padata.c:1142:25: sparse:     got struct parallel_data [noderef] __rcu *pd
   kernel/padata.c: note: in included file (through include/linux/swait.h, include/linux/completion.h):
   include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true
   include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true
   include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true
   include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true
   include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true
   include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true

vim +1142 kernel/padata.c

  1126	
  1127	/**
  1128	 * padata_free_shell - free a padata shell
  1129	 *
  1130	 * @ps: padata shell to free
  1131	 */
  1132	void padata_free_shell(struct padata_shell *ps)
  1133	{
  1134		struct parallel_data *pd;
  1135	
  1136		if (!ps)
  1137			return;
  1138	
  1139		mutex_lock(&ps->pinst->lock);
  1140		list_del(&ps->list);
  1141		pd = rcu_dereference_protected(ps->pd, 1);
> 1142		padata_put_pd(ps->pd);
  1143		mutex_unlock(&ps->pinst->lock);
  1144	
  1145		kfree(ps);
  1146	}
  1147	EXPORT_SYMBOL(padata_free_shell);
  1148	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[chenridong]

On 2024/11/23 16:05, 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>

Friendly ping.

I am looking forward to someone reviewing these patches, and if there
are any opinions, I will update the patch, as well as fixing the compile
warnings.

Thanks,
Ridong

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

[Daniel Jordan]

On Sat, Nov 23, 2024 at 08:05:08AM +0000, Chen Ridong wrote:
> From: Chen Ridong <chenridong@huawei.com>
> 
> Add helpers for pd to get/put refcnt to make code consice.

Seems reasonable.

> +static inline void padata_put_pd(struct parallel_data *pd)
> +{
> +	if (refcount_dec_and_test(&pd->refcnt))
> +		padata_free_pd(pd);
> +}
> +
> +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);
> +}

padata_put_pd could be defined as padata_put_pd_cnt(pd, 1).

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

[chenridong]

On 2024/12/6 7:03, Daniel Jordan wrote:
> On Sat, Nov 23, 2024 at 08:05:08AM +0000, Chen Ridong wrote:
>> From: Chen Ridong <chenridong@huawei.com>
>>
>> Add helpers for pd to get/put refcnt to make code consice.
> 
> Seems reasonable.
> 
>> +static inline void padata_put_pd(struct parallel_data *pd)
>> +{
>> +	if (refcount_dec_and_test(&pd->refcnt))
>> +		padata_free_pd(pd);
>> +}
>> +
>> +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);
>> +}
> 
> padata_put_pd could be defined as padata_put_pd_cnt(pd, 1).
> 

Thank you, will update.

Best regards,
Ridong

[PATCH 2/2] 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, To fix this issue, add refcnt in
the padata_reorder to avoid UAF.

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 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/padata.c b/kernel/padata.c
index 5d8e18cdcb25..627014825266 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -319,6 +319,7 @@ static void padata_reorder(struct parallel_data *pd)
 	if (!spin_trylock_bh(&pd->lock))
 		return;
 
+	padata_get_pd(pd);
 	while (1) {
 		padata = padata_find_next(pd, true);
 
@@ -355,6 +356,7 @@ static void padata_reorder(struct parallel_data *pd)
 	reorder = per_cpu_ptr(pd->reorder_list, pd->cpu);
 	if (!list_empty(&reorder->list) && padata_find_next(pd, false))
 		queue_work(pinst->serial_wq, &pd->reorder_work);
+	padata_put_pd(pd);
 }
 
 static void invoke_padata_reorder(struct work_struct *work)
-- 
2.34.1

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

[Daniel Jordan]

Hello Ridong,

On Sat, Nov 23, 2024 at 08:05:09AM +0000, Chen Ridong wrote:
> From: Chen Ridong <chenridong@huawei.com>
> 
> A bug was found when run ltp test:
...snip...
> 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
> }

Thanks for the fix and clear explanation.

> diff --git a/kernel/padata.c b/kernel/padata.c
> index 5d8e18cdcb25..627014825266 100644
> --- a/kernel/padata.c
> +++ b/kernel/padata.c
> @@ -319,6 +319,7 @@ static void padata_reorder(struct parallel_data *pd)
>  	if (!spin_trylock_bh(&pd->lock))
>  		return;
>  
> +	padata_get_pd(pd);
>  	while (1) {
>  		padata = padata_find_next(pd, true);
>  
> @@ -355,6 +356,7 @@ static void padata_reorder(struct parallel_data *pd)
>  	reorder = per_cpu_ptr(pd->reorder_list, pd->cpu);
>  	if (!list_empty(&reorder->list) && padata_find_next(pd, false))
>  		queue_work(pinst->serial_wq, &pd->reorder_work);
> +	padata_put_pd(pd);

Putting the ref unconditionally here doesn't cover the case where reorder_work
is queued and accesses the freed pd.

The review of patches 3-5 from this series has a potential solution for
this that also keeps some of these refcount operations out of the fast
path:

    https://lore.kernel.org/all/20221019083708.27138-1-nstange@suse.de/

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

[chenridong]

On 2024/12/6 7:01, Daniel Jordan wrote:
> Hello Ridong,
> 
> On Sat, Nov 23, 2024 at 08:05:09AM +0000, Chen Ridong wrote:
>> From: Chen Ridong <chenridong@huawei.com>
>>
>> A bug was found when run ltp test:
> ...snip...
>> 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
>> }
> 
> Thanks for the fix and clear explanation.
> 
>> diff --git a/kernel/padata.c b/kernel/padata.c
>> index 5d8e18cdcb25..627014825266 100644
>> --- a/kernel/padata.c
>> +++ b/kernel/padata.c
>> @@ -319,6 +319,7 @@ static void padata_reorder(struct parallel_data *pd)
>>  	if (!spin_trylock_bh(&pd->lock))
>>  		return;
>>  
>> +	padata_get_pd(pd);
>>  	while (1) {
>>  		padata = padata_find_next(pd, true);
>>  
>> @@ -355,6 +356,7 @@ static void padata_reorder(struct parallel_data *pd)
>>  	reorder = per_cpu_ptr(pd->reorder_list, pd->cpu);
>>  	if (!list_empty(&reorder->list) && padata_find_next(pd, false))
>>  		queue_work(pinst->serial_wq, &pd->reorder_work);
>> +	padata_put_pd(pd);
> 
> Putting the ref unconditionally here doesn't cover the case where reorder_work
> is queued and accesses the freed pd.
> 
> The review of patches 3-5 from this series has a potential solution for
> this that also keeps some of these refcount operations out of the fast
> path:
> 
>     https://lore.kernel.org/all/20221019083708.27138-1-nstange@suse.de/
> 

Thank you for your review.

IIUC, patches 3-5 from this series aim to fix two issue.
1. Avoid UAF for pd(the patch 3).
2. Avoid UAF for ps(the patch 4-5).
What my patch 2 intends to fix is the issue 1.

Let's focus on issue 1.
As shown bellow, if reorder_work is queued, the refcnt must greater than
0, since its serial work have not be finished yet. Do your agree with that?

pcrypt_aead_encrypt/pcrypt_aead_decrypt
padata_do_parallel 			// refcount_inc(&pd->refcnt);
padata_parallel_worker	
padata->parallel(padata);
padata_do_serial(padata);		
// pd->reorder_list 			// enque reorder_list
padata_reorder
 - case1:squeue->work
	padata_serial_worker		// sub refcnt cnt
 - case2:pd->reorder_work		// reorder->list is not empty
	invoke_padata_reorder 		// this means refcnt > 0
	...
	padata_serial_worker

I think the patch 3(from Nicolai Stange) can also avoid UAF for pd, but
it's complicated. IIUC, the issue 1 can only occur in the scenario what
I mentioned is my commit message. How I fix issue 1 is by adding and
putting the refcnt in the padata_reorder function, which is simple and
clear.

If understand something uncorrectly, please let me know.

As the issue 2, I have not encountered it, but it exists in theory.

Thanks,
Ridong

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

[Chen Ridong]

On 2024/12/6 11:48, chenridong wrote:
> 
> 
> On 2024/12/6 7:01, Daniel Jordan wrote:
>> Hello Ridong,
>>
>> On Sat, Nov 23, 2024 at 08:05:09AM +0000, Chen Ridong wrote:
>>> From: Chen Ridong <chenridong@huawei.com>
>>>
>>> A bug was found when run ltp test:
>> ...snip...
>>> 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
>>> }
>>
>> Thanks for the fix and clear explanation.
>>
>>> diff --git a/kernel/padata.c b/kernel/padata.c
>>> index 5d8e18cdcb25..627014825266 100644
>>> --- a/kernel/padata.c
>>> +++ b/kernel/padata.c
>>> @@ -319,6 +319,7 @@ static void padata_reorder(struct parallel_data *pd)
>>>  	if (!spin_trylock_bh(&pd->lock))
>>>  		return;
>>>  
>>> +	padata_get_pd(pd);
>>>  	while (1) {
>>>  		padata = padata_find_next(pd, true);
>>>  
>>> @@ -355,6 +356,7 @@ static void padata_reorder(struct parallel_data *pd)
>>>  	reorder = per_cpu_ptr(pd->reorder_list, pd->cpu);
>>>  	if (!list_empty(&reorder->list) && padata_find_next(pd, false))
>>>  		queue_work(pinst->serial_wq, &pd->reorder_work);
>>> +	padata_put_pd(pd);
>>
>> Putting the ref unconditionally here doesn't cover the case where reorder_work
>> is queued and accesses the freed pd.
>>
>> The review of patches 3-5 from this series has a potential solution for
>> this that also keeps some of these refcount operations out of the fast
>> path:
>>
>>     https://lore.kernel.org/all/20221019083708.27138-1-nstange@suse.de/
>>
> 
> Thank you for your review.
> 
> IIUC, patches 3-5 from this series aim to fix two issue.
> 1. Avoid UAF for pd(the patch 3).
> 2. Avoid UAF for ps(the patch 4-5).
> What my patch 2 intends to fix is the issue 1.
> 
> Let's focus on issue 1.
> As shown bellow, if reorder_work is queued, the refcnt must greater than
> 0, since its serial work have not be finished yet. Do your agree with that?
> 
> pcrypt_aead_encrypt/pcrypt_aead_decrypt
> padata_do_parallel 			// refcount_inc(&pd->refcnt);
> padata_parallel_worker	
> padata->parallel(padata);
> padata_do_serial(padata);		
> // pd->reorder_list 			// enque reorder_list
> padata_reorder
>  - case1:squeue->work
> 	padata_serial_worker		// sub refcnt cnt
>  - case2:pd->reorder_work		// reorder->list is not empty
> 	invoke_padata_reorder 		// this means refcnt > 0
> 	...
> 	padata_serial_worker
> 
> I think the patch 3(from Nicolai Stange) can also avoid UAF for pd, but
> it's complicated. IIUC, the issue 1 can only occur in the scenario what
> I mentioned is my commit message. How I fix issue 1 is by adding and
> putting the refcnt in the padata_reorder function, which is simple and
> clear.
> 
> If understand something uncorrectly, please let me know.
> 
> As the issue 2, I have not encountered it, but it exists in theory.
> 
> Thanks,
> Ridong

Hi, Daniel, I am trying to produce the issue 2. However，I failed.
I added 'mdelay' as helper.

 static void padata_reorder(struct parallel_data *pd)
 {
+       mdelay(10);
        struct padata_instance *pinst = pd->ps->pinst;
        int cb_cpu;
        struct padata_priv *padata;

I believe this can increase the probability of issue 2. But after
testing with pcrypt_aead01, issue 2 cannot be reproduced.
And I don't know whether it exists now.

Looking forward your reply.

Best regards,
Ridong

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

[Daniel Jordan]

On Tue, Dec 10, 2024 at 05:38:51PM +0800, Chen Ridong wrote:
> On 2024/12/6 11:48, chenridong wrote:
> > On 2024/12/6 7:01, Daniel Jordan wrote:
> >> On Sat, Nov 23, 2024 at 08:05:09AM +0000, Chen Ridong wrote:
> > IIUC, patches 3-5 from this series aim to fix two issue.
> > 1. Avoid UAF for pd(the patch 3).
> > 2. Avoid UAF for ps(the patch 4-5).
> > What my patch 2 intends to fix is the issue 1.
...
> Hi, Daniel, I am trying to produce the issue 2. However，I failed.

Thanks for trying!

> I added 'mdelay' as helper.
> 
>  static void padata_reorder(struct parallel_data *pd)
>  {
> +       mdelay(10);
>         struct padata_instance *pinst = pd->ps->pinst;
>         int cb_cpu;
>         struct padata_priv *padata;
> 
> I believe this can increase the probability of issue 2. But after
> testing with pcrypt_aead01, issue 2 cannot be reproduced.
> And I don't know whether it exists now.

Yeah, no reports of issue 2 that I've seen.

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

[Daniel Jordan]

Hi Ridong,

On Fri, Dec 06, 2024 at 11:48:36AM +0800, chenridong wrote:
> On 2024/12/6 7:01, Daniel Jordan wrote:
> > On Sat, Nov 23, 2024 at 08:05:09AM +0000, Chen Ridong wrote:
> >> diff --git a/kernel/padata.c b/kernel/padata.c
> >> index 5d8e18cdcb25..627014825266 100644
> >> --- a/kernel/padata.c
> >> +++ b/kernel/padata.c
> >> @@ -319,6 +319,7 @@ static void padata_reorder(struct parallel_data *pd)
> >>  	if (!spin_trylock_bh(&pd->lock))
> >>  		return;
> >>  
> >> +	padata_get_pd(pd);
> >>  	while (1) {
> >>  		padata = padata_find_next(pd, true);
> >>  
> >> @@ -355,6 +356,7 @@ static void padata_reorder(struct parallel_data *pd)
> >>  	reorder = per_cpu_ptr(pd->reorder_list, pd->cpu);
> >>  	if (!list_empty(&reorder->list) && padata_find_next(pd, false))
> >>  		queue_work(pinst->serial_wq, &pd->reorder_work);
> >> +	padata_put_pd(pd);
> > 
> > Putting the ref unconditionally here doesn't cover the case where reorder_work
> > is queued and accesses the freed pd.
> > 
> > The review of patches 3-5 from this series has a potential solution for
> > this that also keeps some of these refcount operations out of the fast
> > path:
> > 
> >     https://lore.kernel.org/all/20221019083708.27138-1-nstange@suse.de/
> > 
> 
> Thank you for your review.
> 
> IIUC, patches 3-5 from this series aim to fix two issue.
> 1. Avoid UAF for pd(the patch 3).
> 2. Avoid UAF for ps(the patch 4-5).
> What my patch 2 intends to fix is the issue 1.
> 
> Let's focus on issue 1.
> As shown bellow, if reorder_work is queued, the refcnt must greater than
> 0, since its serial work have not be finished yet. Do your agree with that?

I think it's possible for reorder_work to be queued even though all
serial works have finished:

 - padata_reorder finds the reorder list nonempty and sees an object from
   padata_find_next, then gets preempted
 - the serial work finishes in another context
 - back in padata_reorder, reorder_work is queued

Not sure this race could actually happen in practice though.

But, I also think reorder_work can be queued when there's an unfinished
serial work, as you say, but with UAF still happening:

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

> pcrypt_aead_encrypt/pcrypt_aead_decrypt
> padata_do_parallel 			// refcount_inc(&pd->refcnt);
> padata_parallel_worker	
> padata->parallel(padata);
> padata_do_serial(padata);		
> // pd->reorder_list 			// enque reorder_list
> padata_reorder
>  - case1:squeue->work
> 	padata_serial_worker		// sub refcnt cnt
>  - case2:pd->reorder_work		// reorder->list is not empty
> 	invoke_padata_reorder 		// this means refcnt > 0
> 	...
> 	padata_serial_worker

In other words, in case2 above, reorder_work could be queued, another
context could complete the request in padata_serial_worker, and then
invoke_padata_reorder could run and UAF when there aren't any remaining
serial works.

> I think the patch 3(from Nicolai Stange) can also avoid UAF for pd, but
> it's complicated.

For fixing the issue you describe, without regard for the reorder work,
I think the synchronize_rcu from near the end of the patch 3 thread is
enough.  A synchronize_rcu in the slow path seems better than two
atomics in the fast path.

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

[Chen Ridong]

On 2024/12/11 3:12, Daniel Jordan wrote:
> Hi Ridong,
> 
> On Fri, Dec 06, 2024 at 11:48:36AM +0800, chenridong wrote:
>> On 2024/12/6 7:01, Daniel Jordan wrote:
>>> On Sat, Nov 23, 2024 at 08:05:09AM +0000, Chen Ridong wrote:
>>>> diff --git a/kernel/padata.c b/kernel/padata.c
>>>> index 5d8e18cdcb25..627014825266 100644
>>>> --- a/kernel/padata.c
>>>> +++ b/kernel/padata.c
>>>> @@ -319,6 +319,7 @@ static void padata_reorder(struct parallel_data *pd)
>>>>  	if (!spin_trylock_bh(&pd->lock))
>>>>  		return;
>>>>  
>>>> +	padata_get_pd(pd);
>>>>  	while (1) {
>>>>  		padata = padata_find_next(pd, true);
>>>>  
>>>> @@ -355,6 +356,7 @@ static void padata_reorder(struct parallel_data *pd)
>>>>  	reorder = per_cpu_ptr(pd->reorder_list, pd->cpu);
>>>>  	if (!list_empty(&reorder->list) && padata_find_next(pd, false))
>>>>  		queue_work(pinst->serial_wq, &pd->reorder_work);
>>>> +	padata_put_pd(pd);
>>>
>>> Putting the ref unconditionally here doesn't cover the case where reorder_work
>>> is queued and accesses the freed pd.
>>>
>>> The review of patches 3-5 from this series has a potential solution for
>>> this that also keeps some of these refcount operations out of the fast
>>> path:
>>>
>>>     https://lore.kernel.org/all/20221019083708.27138-1-nstange@suse.de/
>>>
>>
>> Thank you for your review.
>>
>> IIUC, patches 3-5 from this series aim to fix two issue.
>> 1. Avoid UAF for pd(the patch 3).
>> 2. Avoid UAF for ps(the patch 4-5).
>> What my patch 2 intends to fix is the issue 1.
>>
>> Let's focus on issue 1.
>> As shown bellow, if reorder_work is queued, the refcnt must greater than
>> 0, since its serial work have not be finished yet. Do your agree with that?
> 
> I think it's possible for reorder_work to be queued even though all
> serial works have finished:
> 
>  - padata_reorder finds the reorder list nonempty and sees an object from
>    padata_find_next, then gets preempted
>  - the serial work finishes in another context
>  - back in padata_reorder, reorder_work is queued
> 
> Not sure this race could actually happen in practice though.
> 
> But, I also think reorder_work can be queued when there's an unfinished
> serial work, as you say, but with UAF still happening:
> 
> 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
> 

Sorry for being busy with other work for a while.
Thank you for your patience.
In theory, it does exist. Although I was unable reproduce it(I added
delay helper as below), I noticed that Herbert has reported a UAF issue
occurred in the padata_parallel_worker function. Therefore, it would be
better to fix it in Nicolai's approach.

static void padata_parallel_worker(struct work_struct *parallel_work)
 {
+       mdelay(10);
+

Hi, Nicolai, would you resend the patch 3 to fix this issue?
I noticed you sent the patch 2 years ago, but this series has not been
merged.

Or may I send a patch that aligns with your approach to resolve it?
Looking forward your feedback.


>> pcrypt_aead_encrypt/pcrypt_aead_decrypt
>> padata_do_parallel 			// refcount_inc(&pd->refcnt);
>> padata_parallel_worker	
>> padata->parallel(padata);
>> padata_do_serial(padata);		
>> // pd->reorder_list 			// enque reorder_list
>> padata_reorder
>>  - case1:squeue->work
>> 	padata_serial_worker		// sub refcnt cnt
>>  - case2:pd->reorder_work		// reorder->list is not empty
>> 	invoke_padata_reorder 		// this means refcnt > 0
>> 	...
>> 	padata_serial_worker
> 
> In other words, in case2 above, reorder_work could be queued, another
> context could complete the request in padata_serial_worker, and then
> invoke_padata_reorder could run and UAF when there aren't any remaining
> serial works.
> 
>> I think the patch 3(from Nicolai Stange) can also avoid UAF for pd, but
>> it's complicated.
> 
> For fixing the issue you describe, without regard for the reorder work,
> I think the synchronize_rcu from near the end of the patch 3 thread is
> enough.  A synchronize_rcu in the slow path seems better than two
> atomics in the fast path.

Thank you. I tested with 'synchronize_rcu', and it can fix the issue I
encountered. As I mentioned, Herbert has provided another stack, which
indicates that case 2 exists. I think it would be better to fix it as
patch 3 did.

Thanks,
Ridong

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

[Daniel Jordan]

Hello Ridong,

On Mon, Dec 23, 2024 at 05:00:16PM +0800, Chen Ridong wrote:
> Sorry for being busy with other work for a while.
> Thank you for your patience.
> In theory, it does exist. Although I was unable reproduce it(I added
> delay helper as below), I noticed that Herbert has reported a UAF issue
> occurred in the padata_parallel_worker function. Therefore, it would be

I'm thinking you're referring to this?:
    https://lore.kernel.org/all/ZuFxD90UO8HadnCj@gondor.apana.org.au/

> better to fix it in Nicolai's approach.
> 
> static void padata_parallel_worker(struct work_struct *parallel_work)
>  {
> +       mdelay(10);
> +
> 
> Hi, Nicolai, would you resend the patch 3 to fix this issue?
> I noticed you sent the patch 2 years ago, but this series has not been
> merged.
> 
> Or may I send a patch that aligns with your approach to resolve it?
> Looking forward your feedback.
> 
> 
> >> pcrypt_aead_encrypt/pcrypt_aead_decrypt
> >> padata_do_parallel 			// refcount_inc(&pd->refcnt);
> >> padata_parallel_worker	
> >> padata->parallel(padata);
> >> padata_do_serial(padata);		
> >> // pd->reorder_list 			// enque reorder_list
> >> padata_reorder
> >>  - case1:squeue->work
> >> 	padata_serial_worker		// sub refcnt cnt
> >>  - case2:pd->reorder_work		// reorder->list is not empty
> >> 	invoke_padata_reorder 		// this means refcnt > 0
> >> 	...
> >> 	padata_serial_worker
> > 
> > In other words, in case2 above, reorder_work could be queued, another
> > context could complete the request in padata_serial_worker, and then
> > invoke_padata_reorder could run and UAF when there aren't any remaining
> > serial works.
> > 
> >> I think the patch 3(from Nicolai Stange) can also avoid UAF for pd, but
> >> it's complicated.
> > 
> > For fixing the issue you describe, without regard for the reorder work,
> > I think the synchronize_rcu from near the end of the patch 3 thread is
> > enough.  A synchronize_rcu in the slow path seems better than two
> > atomics in the fast path.
> 
> Thank you. I tested with 'synchronize_rcu', and it can fix the issue I

Good to know the synchronize_rcu works, thanks for testing that.

> encountered. As I mentioned, Herbert has provided another stack, which
> indicates that case 2 exists. I think it would be better to fix it as
> patch 3 did.

But Nicolai and I already agreed to the synchronize_rcu change plus the
alternative fix in the patch 5 thread:
    https://lore.kernel.org/all/87bkpgb7q6.fsf@suse.de/

These two changes fix all known padata lifetime issues, including the
one with reorder_work in case 2, and keep more refcnt ops out of the
fast path than the original patch 3.

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

[Chen Ridong]

On 2025/1/8 2:43, Daniel Jordan wrote:
> Hello Ridong,
> 
> On Mon, Dec 23, 2024 at 05:00:16PM +0800, Chen Ridong wrote:
>> Sorry for being busy with other work for a while.
>> Thank you for your patience.
>> In theory, it does exist. Although I was unable reproduce it(I added
>> delay helper as below), I noticed that Herbert has reported a UAF issue
>> occurred in the padata_parallel_worker function. Therefore, it would be
> 
> I'm thinking you're referring to this?:
>     https://lore.kernel.org/all/ZuFxD90UO8HadnCj@gondor.apana.org.au/
> 
Yes.

>> better to fix it in Nicolai's approach.
>>
>> static void padata_parallel_worker(struct work_struct *parallel_work)
>>  {
>> +       mdelay(10);
>> +
>>
>> Hi, Nicolai, would you resend the patch 3 to fix this issue?
>> I noticed you sent the patch 2 years ago, but this series has not been
>> merged.
>>
>> Or may I send a patch that aligns with your approach to resolve it?
>> Looking forward your feedback.
>>
>>
>>>> pcrypt_aead_encrypt/pcrypt_aead_decrypt
>>>> padata_do_parallel 			// refcount_inc(&pd->refcnt);
>>>> padata_parallel_worker	
>>>> padata->parallel(padata);
>>>> padata_do_serial(padata);		
>>>> // pd->reorder_list 			// enque reorder_list
>>>> padata_reorder
>>>>  - case1:squeue->work
>>>> 	padata_serial_worker		// sub refcnt cnt
>>>>  - case2:pd->reorder_work		// reorder->list is not empty
>>>> 	invoke_padata_reorder 		// this means refcnt > 0
>>>> 	...
>>>> 	padata_serial_worker
>>>
>>> In other words, in case2 above, reorder_work could be queued, another
>>> context could complete the request in padata_serial_worker, and then
>>> invoke_padata_reorder could run and UAF when there aren't any remaining
>>> serial works.
>>>
>>>> I think the patch 3(from Nicolai Stange) can also avoid UAF for pd, but
>>>> it's complicated.
>>>
>>> For fixing the issue you describe, without regard for the reorder work,
>>> I think the synchronize_rcu from near the end of the patch 3 thread is
>>> enough.  A synchronize_rcu in the slow path seems better than two
>>> atomics in the fast path.
>>
>> Thank you. I tested with 'synchronize_rcu', and it can fix the issue I
> 
> Good to know the synchronize_rcu works, thanks for testing that.
> 
>> encountered. As I mentioned, Herbert has provided another stack, which
>> indicates that case 2 exists. I think it would be better to fix it as
>> patch 3 did.
> 
> But Nicolai and I already agreed to the synchronize_rcu change plus the
> alternative fix in the patch 5 thread:
>     https://lore.kernel.org/all/87bkpgb7q6.fsf@suse.de/
> 
> These two changes fix all known padata lifetime issues, including the
> one with reorder_work in case 2, and keep more refcnt ops out of the
> fast path than the original patch 3.
> 

Thank you. I will send a new series with thought.

Best regard,
Ridong