[PATCH] PM: hibernate: fix crashes with init_on_free=1

[PATCH] PM: hibernate: fix crashes with init_on_free=1

[glider]

Upon resuming from hibernation, free pages may contain stale data from
the kernel that initiated the resume. This breaks the invariant
inflicted by init_on_free=1 that freed pages must be zeroed.

To deal with this problem, make clear_free_pages() also clear the free
pages when init_on_free is enabled.

Reported-by: Johannes Stezenbach <js@sig21.net>
Signed-off-by: Alexander Potapenko <glider@google.com>
Fixes: 6471384af2a6 ("mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options")
---
 kernel/power/snapshot.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 26b9168321e7..d65f2d5ab694 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1147,24 +1147,24 @@ void free_basic_memory_bitmaps(void)
 
 void clear_free_pages(void)
 {
-#ifdef CONFIG_PAGE_POISONING_ZERO
 	struct memory_bitmap *bm = free_pages_map;
 	unsigned long pfn;
 
 	if (WARN_ON(!(free_pages_map)))
 		return;
 
-	memory_bm_position_reset(bm);
-	pfn = memory_bm_next_pfn(bm);
-	while (pfn != BM_END_OF_MAP) {
-		if (pfn_valid(pfn))
-			clear_highpage(pfn_to_page(pfn));
-
+	if (IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) || want_init_on_free()) {
+		memory_bm_position_reset(bm);
 		pfn = memory_bm_next_pfn(bm);
+		while (pfn != BM_END_OF_MAP) {
+			if (pfn_valid(pfn))
+				clear_highpage(pfn_to_page(pfn));
+
+			pfn = memory_bm_next_pfn(bm);
+		}
+		memory_bm_position_reset(bm);
+		pr_info("free pages cleared after restore\n");
 	}
-	memory_bm_position_reset(bm);
-	pr_info("free pages cleared after restore\n");
-#endif /* PAGE_POISONING_ZERO */
 }
 
 /**
-- 
2.25.0.rc1.283.g88dfdc4193-goog

Re: [PATCH] PM: hibernate: fix crashes with init_on_free=1

[Rafael J. Wysocki]

On Thu, Jan 16, 2020 at 12:10 PM <glider@google.com> wrote:
>
> Upon resuming from hibernation, free pages may contain stale data from
> the kernel that initiated the resume. This breaks the invariant
> inflicted by init_on_free=1 that freed pages must be zeroed.
>
> To deal with this problem, make clear_free_pages() also clear the free
> pages when init_on_free is enabled.
>
> Reported-by: Johannes Stezenbach <js@sig21.net>
> Signed-off-by: Alexander Potapenko <glider@google.com>
> Fixes: 6471384af2a6 ("mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options")
> ---
>  kernel/power/snapshot.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index 26b9168321e7..d65f2d5ab694 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -1147,24 +1147,24 @@ void free_basic_memory_bitmaps(void)
>
>  void clear_free_pages(void)
>  {
> -#ifdef CONFIG_PAGE_POISONING_ZERO
>         struct memory_bitmap *bm = free_pages_map;
>         unsigned long pfn;
>
>         if (WARN_ON(!(free_pages_map)))
>                 return;
>
> -       memory_bm_position_reset(bm);
> -       pfn = memory_bm_next_pfn(bm);
> -       while (pfn != BM_END_OF_MAP) {
> -               if (pfn_valid(pfn))
> -                       clear_highpage(pfn_to_page(pfn));
> -
> +       if (IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) || want_init_on_free()) {
> +               memory_bm_position_reset(bm);
>                 pfn = memory_bm_next_pfn(bm);
> +               while (pfn != BM_END_OF_MAP) {
> +                       if (pfn_valid(pfn))
> +                               clear_highpage(pfn_to_page(pfn));
> +
> +                       pfn = memory_bm_next_pfn(bm);
> +               }
> +               memory_bm_position_reset(bm);
> +               pr_info("free pages cleared after restore\n");
>         }
> -       memory_bm_position_reset(bm);
> -       pr_info("free pages cleared after restore\n");
> -#endif /* PAGE_POISONING_ZERO */
>  }
>
>  /**
> --

Queued up as a fix for 5.5-rc and "stable", thanks!

Re: [PATCH] PM: hibernate: fix crashes with init_on_free=1

[Alexander Potapenko]

On Thu, Jan 16, 2020 at 11:56 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Jan 16, 2020 at 12:10 PM <glider@google.com> wrote:
> >
> > Upon resuming from hibernation, free pages may contain stale data from
> > the kernel that initiated the resume. This breaks the invariant
> > inflicted by init_on_free=1 that freed pages must be zeroed.
> >
> > To deal with this problem, make clear_free_pages() also clear the free
> > pages when init_on_free is enabled.
> >
> > Reported-by: Johannes Stezenbach <js@sig21.net>
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> > Fixes: 6471384af2a6 ("mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options")
> > ---
> >  kernel/power/snapshot.c | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> > index 26b9168321e7..d65f2d5ab694 100644
> > --- a/kernel/power/snapshot.c
> > +++ b/kernel/power/snapshot.c
> > @@ -1147,24 +1147,24 @@ void free_basic_memory_bitmaps(void)
> >
> >  void clear_free_pages(void)
> >  {
> > -#ifdef CONFIG_PAGE_POISONING_ZERO
> >         struct memory_bitmap *bm = free_pages_map;
> >         unsigned long pfn;
> >
> >         if (WARN_ON(!(free_pages_map)))
> >                 return;
> >
> > -       memory_bm_position_reset(bm);
> > -       pfn = memory_bm_next_pfn(bm);
> > -       while (pfn != BM_END_OF_MAP) {
> > -               if (pfn_valid(pfn))
> > -                       clear_highpage(pfn_to_page(pfn));
> > -
> > +       if (IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) || want_init_on_free()) {
> > +               memory_bm_position_reset(bm);
> >                 pfn = memory_bm_next_pfn(bm);
> > +               while (pfn != BM_END_OF_MAP) {
> > +                       if (pfn_valid(pfn))
> > +                               clear_highpage(pfn_to_page(pfn));
> > +
> > +                       pfn = memory_bm_next_pfn(bm);
> > +               }
> > +               memory_bm_position_reset(bm);
> > +               pr_info("free pages cleared after restore\n");
> >         }
> > -       memory_bm_position_reset(bm);
> > -       pr_info("free pages cleared after restore\n");
> > -#endif /* PAGE_POISONING_ZERO */
> >  }
> >
> >  /**
> > --
>
> Queued up as a fix for 5.5-rc and "stable", thanks!

It's actually interesting that init_on_free=1 has introduced an
invariant that pages in page heap must be initialized with zeros.
Code in mm/page_alloc.c relies on this invariant when allocating with
__GFP_ZERO, so if there's a page that somehow ends up uninitialized,
we get all sorts of vulnerabilities.
We were lucky to see this firing in hibernate, and I've grepped the
code for other occurrences of CONFIG_PAGE_POISONING_ZERO, but I'm not
sure we are not missing any other ways to put an uninitialized page
into the page heap.


-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg