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

Chen Ridong
2025-01-09T07:27:06Z

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