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: <SJ0PR11MB5678C6C693444F64E38CE2EBC9202@SJ0PR11MB5678.namprd11.prod.outlook.com>
Date: Tue, 19 Nov 2024 19:22:36 +0000
From: "Sridhar, Kanchana P" <kanchana.p.sridhar@...el.com>
To: Yosry Ahmed <yosryahmed@...gle.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>, "Sridhar, Kanchana P"
	<kanchana.p.sridhar@...el.com>
Subject: RE: [PATCH v1] mm: zswap: Fix a potential memory leak in
 zswap_decompress().


> -----Original Message-----
> From: Yosry Ahmed <yosryahmed@...gle.com>
> Sent: Friday, November 15, 2024 1:49 PM
> 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-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().
> 
> 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.

Would it be sufficient to add a check for mutex_is_locked() in
zswap_cpu_comp_dead() and if this returns true, to exit without deleting
the acomp? If this is an acceptable solution, it would also require us
to move the mutex_unlock() to occur after the "if (src != acomp_ctx->buffer)"
in zswap_decompress(). This would ensure all existing zswap code that's
within the mutex_lock()-mutex_unlock() will work correctly without
worrying about the acomp_ctx being deleted by cpu offlining.

Not sure if this would be a comprehensive solution, or if it would have
unintended consequences to the cpu offlining code. Would appreciate
comments.

Thanks,
Kanchana

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