Re: [PATCH v1] mm: zswap: Fix a potential memory leak in zswap_decompress().

[Yosry Ahmed <yosryahmed@google.com>]

On Tue, Nov 19, 2024 at 11:22 AM Sridhar, Kanchana P
<kanchana.p.sridhar@intel.com> wrote:
>
>
> > -----Original Message-----
> > From: Yosry Ahmed <yosryahmed@google.com>
> > Sent: Friday, November 15, 2024 1:49 PM
> > To: Sridhar, Kanchana P <kanchana.p.sridhar@intel.com>
> > Cc: Chengming Zhou <chengming.zhou@linux.dev>; Johannes Weiner
> > <hannes@cmpxchg.org>; Nhat Pham <nphamcs@gmail.com>; linux-
> > kernel@vger.kernel.org; linux-mm@kvack.org; usamaarif642@gmail.com;
> > ryan.roberts@arm.com; Huang, Ying <ying.huang@intel.com>;
> > 21cnbao@gmail.com; akpm@linux-foundation.org; Feghali, Wajdi K
> > <wajdi.k.feghali@intel.com>; Gopal, Vinodh <vinodh.gopal@intel.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@intel.com> wrote:
> > >
> > > Hi Chengming,
> > >
> > > > -----Original Message-----
> > > > From: Chengming Zhou <chengming.zhou@linux.dev>
> > > > Sent: Wednesday, November 13, 2024 11:24 PM
> > > > To: Sridhar, Kanchana P <kanchana.p.sridhar@intel.com>; Johannes
> > Weiner
> > > > <hannes@cmpxchg.org>
> > > > Cc: Nhat Pham <nphamcs@gmail.com>; Yosry Ahmed
> > > > <yosryahmed@google.com>; linux-kernel@vger.kernel.org; linux-
> > > > mm@kvack.org; usamaarif642@gmail.com; ryan.roberts@arm.com;
> > Huang,
> > > > Ying <ying.huang@intel.com>; 21cnbao@gmail.com; akpm@linux-
> > > > foundation.org; Feghali, Wajdi K <wajdi.k.feghali@intel.com>; Gopal,
> > Vinodh
> > > > <vinodh.gopal@intel.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@cmpxchg.org>
> > > > >> Sent: Wednesday, November 13, 2024 9:12 PM
> > > > >> To: Sridhar, Kanchana P <kanchana.p.sridhar@intel.com>
> > > > >> Cc: Nhat Pham <nphamcs@gmail.com>; Yosry Ahmed
> > > > >> <yosryahmed@google.com>; linux-kernel@vger.kernel.org; linux-
> > > > >> mm@kvack.org; chengming.zhou@linux.dev;
> > usamaarif642@gmail.com;
> > > > >> ryan.roberts@arm.com; Huang, Ying <ying.huang@intel.com>;
> > > > >> 21cnbao@gmail.com; akpm@linux-foundation.org; Feghali, Wajdi K
> > > > >> <wajdi.k.feghali@intel.com>; Gopal, Vinodh <vinodh.gopal@intel.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.

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.

> 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