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: <jfjz5d7zwbytztackem7ibzalm5lnxldi2eofeiczqmqs2m7o6@fq426cwnjtkm>
Date: Tue, 7 Jan 2025 13:43:15 -0500
From: Daniel Jordan <daniel.m.jordan@...cle.com>
To: Chen Ridong <chenridong@...weicloud.com>
Cc: chenridong <chenridong@...wei.com>, 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

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.