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: <SJ0PR11MB5678A7DF494C3FCB687D620EC9202@SJ0PR11MB5678.namprd11.prod.outlook.com>
Date: Tue, 19 Nov 2024 19:41:46 +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>,
	"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: Tuesday, November 19, 2024 11:27 AM
> 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 Tue, Nov 19, 2024 at 11:22 AM Sridhar, Kanchana P
> <kanchana.p.sridhar@...el.com> wrote:
> >
> >
> > > -----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?
> 
> I don't think this works. First of all, it's racy. It's possible the
> mutex gets locked after we check mutex_is_locked() but before we
> delete the acomp_ctx. Also, if we find that the mutex is locked, then
> we do nothing and essentially leak the memory.

Yes, this would assume the cpu offlining code retries at some interval,
which could prevent the memory leak.

> 
> Second, and probably more important, this only checks if anyone is
> currently holding the mutex. What about tasks that may be sleeping
> waiting for the mutex to be unlocked? The mutex will be deleted from
> under them as well.

Wouldn't this and the race described above, also be issues for the
refcount based approach?

Also, I am wondering if the mutex design already handles cases where
tasks are sleeping, waiting for a mutex that disappears?

Thanks,
Kanchana

> 
> > 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