linux-kernel - Re: [PATCH v1] mm: zswap: Fix a potential memory leak in zswap

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: <CAKEwX=P6mxZ+-5UcunRHeoAVwtZD=UMfKqCGUeun-krJeT8ekg@mail.gmail.com>
Date: Wed, 13 Nov 2024 16:28:40 -0800
From: Nhat Pham <nphamcs@...il.com>
To: "Sridhar, Kanchana P" <kanchana.p.sridhar@...el.com>
Cc: Johannes Weiner <hannes@...xchg.org>, Yosry Ahmed <yosryahmed@...gle.com>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "linux-mm@...ck.org" <linux-mm@...ck.org>, 
	"chengming.zhou@...ux.dev" <chengming.zhou@...ux.dev>, 
	"usamaarif642@...il.com" <usamaarif642@...il.com>, "ryan.roberts@....com" <ryan.roberts@....com>, 
	"Huang, Ying" <ying.huang@...el.com>, "21cnbao@...il.com" <21cnbao@...il.com>, 
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>, "Feghali, Wajdi K" <wajdi.k.feghali@...el.com>, 
	"Gopal, Vinodh" <vinodh.gopal@...el.com>
Subject: Re: [PATCH v1] mm: zswap: Fix a potential memory leak in zswap_decompress().

On Wed, Nov 13, 2024 at 2:13 PM Sridhar, Kanchana P
<kanchana.p.sridhar@...el.com> wrote:
>
>
>
> Thanks Johannes, for these insights. I was thinking of the following
> in zswap_decompress() as creating a non-preemptible context because
> of the call to raw_cpu_ptr() at the start; with this context extending
> until the mutex_unlock():
>
>         acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx);
>         mutex_lock(&acomp_ctx->mutex);
>
>         [...]
>
>         mutex_unlock(&acomp_ctx->mutex);
>
>         if (src != acomp_ctx->buffer)
>                 zpool_unmap_handle(zpool, entry->handle);
>
> Based on this understanding, I was a bit worried about the
> "acomp_ctx->buffer" in the conditional that gates the
> zpool_unmap_handle() not being the same acomp_ctx as the one
> at the beginning. I may have been confusing myself, since the acomp_ctx
> is not re-evaluated before the conditional, just reused from the
> start. My apologies to you and Yosry!
>
> >
> > That being said, I do think there is a UAF bug in CPU hotplugging.
> >
> > There is an acomp_ctx for each cpu, but note that this is best effort
> > parallelism, not a guarantee that we always have the context of the
> > local CPU. Look closely: we pick the "local" CPU with preemption
> > enabled, then contend for the mutex. This may well put us to sleep and
> > get us migrated, so we could be using the context of a CPU we are no
> > longer running on. This is fine because we hold the mutex - if that
> > other CPU tries to use the acomp_ctx, it'll wait for us.
> >
> > However, if we get migrated and vacate the CPU whose context we have
> > locked, the CPU might get offlined and zswap_cpu_comp_dead() can free
> > the context underneath us. I think we need to refcount the acomp_ctx.
>
> I see. Wouldn't it then seem to make the code more fail-safe to not allow
> the migration to happen until after the check for (src != acomp_ctx->buffer), by
> moving the mutex_unlock() after this check? Or, use a boolean to determine
> if the unmap_handle needs to be done as Yosry suggested?

Hmm does it make it safe? It is mutex_lock() that puts the task to
sleep, after which it can get migrated to a different CPU. Moving
mutex_unlock() to below or not doesn't really matter, no? Or am I
missing something here...

I think Johannes' proposal is the safest :)