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: <CAJD7tkZ+OM2uiHgHHeNqBeSaptfXw4MG=E280-5PKpeybB=8dQ@mail.gmail.com>
Date: Fri, 15 Nov 2024 13:49:05 -0800
From: Yosry Ahmed <yosryahmed@...gle.com>
To: "Sridhar, Kanchana P" <kanchana.p.sridhar@...el.com>
Cc: Chengming Zhou <chengming.zhou@...ux.dev>, Johannes Weiner <hannes@...xchg.org>, 
	Nhat Pham <nphamcs@...il.com>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "linux-mm@...ck.org" <linux-mm@...ck.org>, 
	"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 Fri, Nov 15, 2024 at 1:14 PM Sridhar, Kanchana P
<kanchana.p.sridhar@...el.com> wrote:
>
> Hi Chengming,
>
> > -----Original Message-----
> > From: Chengming Zhou <chengming.zhou@...ux.dev>
> > Sent: Wednesday, November 13, 2024 11:24 PM
> > To: Sridhar, Kanchana P <kanchana.p.sridhar@...el.com>; Johannes Weiner
> > <hannes@...xchg.org>
> > Cc: Nhat Pham <nphamcs@...il.com>; Yosry Ahmed
> > <yosryahmed@...gle.com>; linux-kernel@...r.kernel.org; linux-
> > mm@...ck.org; usamaarif642@...il.com; ryan.roberts@....com; Huang,
> > Ying <ying.huang@...el.com>; 21cnbao@...il.com; 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().
> >
> > Hello,
> >
> > On 2024/11/14 14:37, Sridhar, Kanchana P wrote:
> > >
> > >> -----Original Message-----
> > >> From: Johannes Weiner <hannes@...xchg.org>
> > >> Sent: Wednesday, November 13, 2024 9:12 PM
> > >> To: Sridhar, Kanchana P <kanchana.p.sridhar@...el.com>
> > >> Cc: Nhat Pham <nphamcs@...il.com>; Yosry Ahmed
> > >> <yosryahmed@...gle.com>; linux-kernel@...r.kernel.org; linux-
> > >> mm@...ck.org; chengming.zhou@...ux.dev; usamaarif642@...il.com;
> > >> ryan.roberts@....com; Huang, Ying <ying.huang@...el.com>;
> > >> 21cnbao@...il.com; 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 Thu, Nov 14, 2024 at 01:56:16AM +0000, Sridhar, Kanchana P wrote:
> > >>> So my question was, can we prevent the migration to a different cpu
> > >>> by relinquishing the mutex lock after this conditional
> > >>
> > >> Holding the mutex doesn't prevent preemption/migration.
> > >
> > > Sure, however, is this also applicable to holding the mutex of a per-cpu
> > > structure obtained via raw_cpu_ptr()?
> >
> > Yes, unless you use migration_disable() or cpus_read_lock() to protect
> > this section.
>
> Ok.
>
> >
> > >
> > > Would holding the mutex prevent the acomp_ctx of the cpu prior to
> > > the migration (in the UAF scenario you described) from being deleted?
> >
> > No, cpu offline can kick in anytime to free the acomp_ctx->buffer.
> >
> > >
> > > If holding the per-cpu acomp_ctx's mutex isn't sufficient to prevent the
> > > UAF, I agree, we might need a way to prevent the acomp_ctx from being
> > > deleted, e.g. with refcounts as you've suggested, or to not use the
> >
> > Right, refcount solution from Johannes is very good IMHO.
> >
> > > acomp_ctx at all for the check, instead use a boolean.
> >
> > But this is not enough to just avoid using acomp_ctx for the check,
> > the usage of acomp_ctx inside the mutex is also UAF, since cpu offline
> > can kick in anytime to free the acomp_ctx->buffer.
>
> I see. How would the refcounts work? Would this add latency to zswap
> ops? In low memory situations, could the cpu offlining code over-ride
> the refcounts?

I think what Johannes meant is that the zswap compress/decompress
paths grab a ref on the acomp_ctx before using it, and the CPU
offlining code only drops the initial ref, and does not free the
buffer directly. The buffer is only freed when the ref drops to zero.

I am not familiar with CPU hotplug, would it be simpler if we have a
wrapper like get_acomp_ctx() that disables migration or calls
cpus_read_lock() before grabbing the per-CPU acomp_ctx? A similar
wrapper, put_acompt_ctx() will be used after we are done using the
acomp_ctx.

>
> Based on Johannes' earlier comments, I don't think it makes sense for
> me to submit a v2.
>
> Thanks,
> Kanchana
>
> >
> > Thanks.