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@xxxxxxxxxxxxxxxxxxx/ > 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@xxxxxxx/ > > 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
