linux-kernel - Re: [PATCH 2/2] padata: fix UAF in padata

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] [day] [month] [year] [list]

Message-ID: <348d0c75-b8a9-4319-93f4-de2f1dd1eab8@huaweicloud.com>
Date: Thu, 9 Jan 2025 15:26:58 +0800
From: Chen Ridong <chenridong@...weicloud.com>
To: Daniel Jordan <daniel.m.jordan@...cle.com>,
 Chen Ridong <chenridong@...weicloud.com>
Cc: nstange@...e.de, steffen.klassert@...unet.com,
 herbert@...dor.apana.org.au, linux-crypto@...r.kernel.org,
 linux-kernel@...r.kernel.org, wangweiyang2@...wei.com
Subject: Re: [PATCH 2/2] padata: fix UAF in padata_reorder



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