linux-kernel - Re: [PATCH] zswap: initialize entry->pool on same filled entry

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 21 Mar 2024 17:41:23 -0700
From: Chris Li <chrisl@...nel.org>
To: Yosry Ahmed <yosryahmed@...gle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, linux-kernel@...r.kernel.org, 
	linux-mm@...ck.org, Nhat Pham <nphamcs@...il.com>, 
	Johannes Weiner <hannes@...xchg.org>, Chengming Zhou <zhouchengming@...edance.com>
Subject: Re: [PATCH] zswap: initialize entry->pool on same filled entry

On Thu, Mar 21, 2024 at 4:56 PM Yosry Ahmed <yosryahmed@...gle.com> wrote:
>
> On Thu, Mar 21, 2024 at 4:53 PM Chris Li <chrisl@...nel.org> wrote:
> >
> > Current zswap will leave the entry->pool uninitialized if
> > the page is same  filled. The entry->pool pointer can
> > contain data written by previous usage.
> >
> > Initialize entry->pool to zero for the same filled zswap entry.
> >
> > Signed-off-by: Chris Li <chrisl@...nel.org>
> > ---
> > Per Yosry's suggestion to split out this clean up
> > from the zxwap rb tree to xarray patch.
> >
> > https://lore.kernel.org/all/ZemDuW25YxjqAjm-@google.com/
> > ---
> >  mm/zswap.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index b31c977f53e9..f04a75a36236 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -1527,6 +1527,7 @@ bool zswap_store(struct folio *folio)
> >                         kunmap_local(src);
> >                         entry->length = 0;
> >                         entry->value = value;
> > +                       entry->pool = 0;
>
> This should be NULL.
>
> That being said, I am working on a series that should make non-filled
> entries not use a zswap_entry at all. So I think this cleanup is
> unnecessary, especially that it is documented in the definition of
> struct zswap_entry that entry->pool is invalid for same-filled
> entries.

It does not really hurt to initialize it. It is obviously correct if
we initialize it as well. One thing to consider is that, this pointer
can contain user space data if the page previously was map to user
space. Kdump typically doesn't save user space data. This
uninitialized value might let kdump contain user space data.

Chris

>
> >                         atomic_inc(&zswap_same_filled_pages);
> >                         goto insert_entry;
> >                 }
> >
> > ---
> > base-commit: a824831a082f1d8f9b51a4c0598e633d38555fcf
> > change-id: 20240315-zswap-fill-f65f44574760
> >
> > Best regards,
> > --
> > Chris Li <chrisl@...nel.org>
> >