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/
> 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.
