linux-kernel - Re: [PATCH] mm: zswap: properly synchronize freeing resources during CPU hotunplug

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

Message-ID: <CAJD7tkY5t96siv10Bbve1W_p7LUkW4iHQj_Gg4cbX0hpQuk7DA@mail.gmail.com>
Date: Wed, 8 Jan 2025 12:50:33 -0800
From: Yosry Ahmed <yosryahmed@...gle.com>
To: "Sridhar, Kanchana P" <kanchana.p.sridhar@...el.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, Johannes Weiner <hannes@...xchg.org>, 
	Nhat Pham <nphamcs@...il.com>, Chengming Zhou <chengming.zhou@...ux.dev>, 
	Vitaly Wool <vitalywool@...il.com>, Barry Song <baohua@...nel.org>, 
	Sam Sun <samsun1006219@...il.com>, "linux-mm@...ck.org" <linux-mm@...ck.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH] mm: zswap: properly synchronize freeing resources during
 CPU hotunplug

On Wed, Jan 8, 2025 at 12:23 PM Sridhar, Kanchana P
<kanchana.p.sridhar@...el.com> wrote:
>
>
> > -----Original Message-----
> > From: Yosry Ahmed <yosryahmed@...gle.com>
> > Sent: Wednesday, January 8, 2025 8:15 AM
> > To: Andrew Morton <akpm@...ux-foundation.org>
> > Cc: Johannes Weiner <hannes@...xchg.org>; Nhat Pham
> > <nphamcs@...il.com>; Chengming Zhou <chengming.zhou@...ux.dev>;
> > Vitaly Wool <vitalywool@...il.com>; Barry Song <baohua@...nel.org>; Sam
> > Sun <samsun1006219@...il.com>; Sridhar, Kanchana P
> > <kanchana.p.sridhar@...el.com>; linux-mm@...ck.org; linux-
> > kernel@...r.kernel.org; Yosry Ahmed <yosryahmed@...gle.com>;
> > stable@...r.kernel.org
> > Subject: [PATCH] mm: zswap: properly synchronize freeing resources during
> > CPU hotunplug
> >
> > In zswap_compress() and zswap_decompress(), the per-CPU acomp_ctx of
> > the
> > current CPU at the beginning of the operation is retrieved and used
> > throughout.  However, since neither preemption nor migration are
> > disabled, it is possible that the operation continues on a different
> > CPU.
> >
> > If the original CPU is hotunplugged while the acomp_ctx is still in use,
> > we run into a UAF bug as some of the resources attached to the acomp_ctx
> > are freed during hotunplug in zswap_cpu_comp_dead().
> >
> > The problem was introduced in commit 1ec3b5fe6eec ("mm/zswap: move to
> > use crypto_acomp API for hardware acceleration") when the switch to the
> > crypto_acomp API was made.  Prior to that, the per-CPU crypto_comp was
> > retrieved using get_cpu_ptr() which disables preemption and makes sure
> > the CPU cannot go away from under us.  Preemption cannot be disabled
> > with the crypto_acomp API as a sleepable context is needed.
> >
> > During CPU hotunplug, hold the acomp_ctx.mutex before freeing any
> > resources, and set acomp_ctx.req to NULL when it is freed. In the
> > compress/decompress paths, after acquiring the acomp_ctx.mutex make sure
> > that acomp_ctx.req is not NULL (i.e. acomp_ctx resources were not freed
> > by CPU hotunplug). Otherwise, retry with the acomp_ctx from the new CPU.
> >
> > This adds proper synchronization to ensure that the acomp_ctx resources
> > are not freed from under compress/decompress paths.
> >
> > Note that the per-CPU acomp_ctx itself (including the mutex) is not
> > freed during CPU hotunplug, only acomp_ctx.req, acomp_ctx.buffer, and
> > acomp_ctx.acomp. So it is safe to acquire the acomp_ctx.mutex of a CPU
> > after it is hotunplugged.
>
> Only other fail-proofing I can think of is to initialize the mutex right after
> the per-cpu acomp_ctx is allocated in zswap_pool_create() and de-couple
> it from the cpu onlining. This further clarifies the intent for this mutex
> to be used at the same lifetime scope as the acomp_ctx itself, independent
> of cpu hotplug/hotunplug.

I mentioned doing this initially then dismissed it as a readability
improvement. However, I think it's actually required for correctness.
It's possible that the CPU becomes online again after
acomp_ctx_get_cpu_lock() decides to retry but before it unlocks the
mutex, in which case the CPU being onlined will reinitialize an
already locked mutex.

I will add that and send a v2 shortly.