[PATCH] mm/page_alloc: always scrub pages given to the allocator

[PATCH] mm/page_alloc: always scrub pages given to the allocator

[Sergey Dyasli]

Having the allocator return unscrubbed pages is a potential security
concern: some domain can be given pages with memory contents of another
domain. This may happen, for example, if a domain voluntarily releases
its own memory (ballooning being the easiest way for doing this).

Change the allocator to always scrub the pages given to it by:

1. free_xenheap_pages()
2. free_domheap_pages()
3. online_page()
4. init_heap_pages()

Performance testing has shown that on multi-node machines bootscrub
vastly outperforms idle-loop scrubbing. So instead of marking all pages
dirty initially, introduce bootscrub_done to track the completion of
the process and eagerly scrub all allocated pages during boot.

If bootscrub is disabled, then all pages will be marked as dirty right
away and scrubbed either in idle-loop or eagerly during allocation.

After this patch, alloc_heap_pages() is guaranteed to return scrubbed
pages to a caller unless MEMF_no_scrub flag was provided.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien.grall@arm.com>
CC: Tim Deegan <tim@xen.org>
---
 docs/misc/xen-command-line.markdown |  3 ++-
 xen/common/page_alloc.c             | 29 ++++++++++++++---------------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 1ffd586224..d9bebf4e4b 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -233,7 +233,8 @@ Xen's command line.
 
 Scrub free RAM during boot.  This is a safety feature to prevent
 accidentally leaking sensitive VM data into other VMs if Xen crashes
-and reboots.
+and reboots. Note: even if disabled, RAM will still be scrubbed in
+background.
 
 ### bootscrub\_chunk
 > `= <size>`
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 16e1b0c357..cb1c265f9c 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -161,8 +161,9 @@ string_param("badpage", opt_badpage);
 /*
  * no-bootscrub -> Free pages are not zeroed during boot.
  */
-static bool_t opt_bootscrub __initdata = 1;
+static bool __read_mostly opt_bootscrub = true;
 boolean_param("bootscrub", opt_bootscrub);
+static bool __read_mostly bootscrub_done;
 
 /*
  * bootscrub_chunk -> Amount of bytes to scrub lockstep on non-SMT CPUs
@@ -1026,6 +1027,12 @@ static struct page_info *alloc_heap_pages(
         }
     }
 
+    if ( unlikely(opt_bootscrub && !bootscrub_done) )
+    {
+        for ( i = 0; i < (1U << order); i++ )
+            scrub_one_page(&pg[i]);
+    }
+
     if ( need_tlbflush )
         filtered_flush_tlb_mask(tlbflush_timestamp);
 
@@ -1684,7 +1691,7 @@ unsigned int online_page(unsigned long mfn, uint32_t *status)
     spin_unlock(&heap_lock);
 
     if ( (y & PGC_state) == PGC_state_offlined )
-        free_heap_pages(pg, 0, false);
+        free_heap_pages(pg, 0, true);
 
     return ret;
 }
@@ -1763,7 +1770,8 @@ static void init_heap_pages(
             nr_pages -= n;
         }
 
-        free_heap_pages(pg + i, 0, scrub_debug);
+        free_heap_pages(pg + i, 0, scrub_debug ||
+                                   (opt_bootscrub ? bootscrub_done : true));
     }
 }
 
@@ -2024,6 +2032,7 @@ static void __init scrub_heap_pages(void)
         }
     }
 
+    bootscrub_done = true;
     printk("done.\n");
 
 #ifdef CONFIG_SCRUB_DEBUG
@@ -2098,7 +2107,7 @@ void free_xenheap_pages(void *v, unsigned int order)
 
     memguard_guard_range(v, 1 << (order + PAGE_SHIFT));
 
-    free_heap_pages(virt_to_page(v), order, false);
+    free_heap_pages(virt_to_page(v), order, true);
 }
 
 #else
@@ -2293,8 +2302,6 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
     }
     else
     {
-        bool_t scrub;
-
         if ( likely(d) && likely(d != dom_cow) )
         {
             /* NB. May recursively lock from relinquish_memory(). */
@@ -2309,13 +2316,6 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
             drop_dom_ref = !domain_adjust_tot_pages(d, -(1 << order));
 
             spin_unlock_recursive(&d->page_alloc_lock);
-
-            /*
-             * Normally we expect a domain to clear pages before freeing them,
-             * if it cares about the secrecy of their contents. However, after
-             * a domain has died we assume responsibility for erasure.
-             */
-            scrub = d->is_dying || scrub_debug;
         }
         else
         {
@@ -2328,10 +2328,9 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
              */
             ASSERT(!d || !order);
             drop_dom_ref = 0;
-            scrub = 1;
         }
 
-        free_heap_pages(pg, order, scrub);
+        free_heap_pages(pg, order, true);
     }
 
     if ( drop_dom_ref )
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm/page_alloc: always scrub pages given to the allocator

[Julien Grall]

Hi,

On 10/01/2018 10:58 AM, Sergey Dyasli wrote:
> Having the allocator return unscrubbed pages is a potential security
> concern: some domain can be given pages with memory contents of another
> domain. This may happen, for example, if a domain voluntarily releases
> its own memory (ballooning being the easiest way for doing this).

Based on the comment you dropped below, I would have thought the guest 
is responsible for scrubbing page it gives back using ballooning. Did I 
miss anything?

> 
> Change the allocator to always scrub the pages given to it by:
> 
> 1. free_xenheap_pages()
> 2. free_domheap_pages()
> 3. online_page()
> 4. init_heap_pages()
> 
> Performance testing has shown that on multi-node machines bootscrub
> vastly outperforms idle-loop scrubbing. So instead of marking all pages
> dirty initially, introduce bootscrub_done to track the completion of
> the process and eagerly scrub all allocated pages during boot.
> 
> If bootscrub is disabled, then all pages will be marked as dirty right
> away and scrubbed either in idle-loop or eagerly during allocation.
> 
> After this patch, alloc_heap_pages() is guaranteed to return scrubbed
> pages to a caller unless MEMF_no_scrub flag was provided.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Tim Deegan <tim@xen.org>
> ---
>   docs/misc/xen-command-line.markdown |  3 ++-
>   xen/common/page_alloc.c             | 29 ++++++++++++++---------------
>   2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index 1ffd586224..d9bebf4e4b 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -233,7 +233,8 @@ Xen's command line.
>   
>   Scrub free RAM during boot.  This is a safety feature to prevent
>   accidentally leaking sensitive VM data into other VMs if Xen crashes
> -and reboots.
> +and reboots. Note: even if disabled, RAM will still be scrubbed in
> +background.
Some of the user may want to skip boot scrub because they don't need it 
or for testing as it is too slow on some platform (i.e models).

I think we still need to give an option to those users. If they disabled 
boot scrub, then they now the security risk based on the description of 
the option.

>   
>   ### bootscrub\_chunk
>   > `= <size>`
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 16e1b0c357..cb1c265f9c 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -161,8 +161,9 @@ string_param("badpage", opt_badpage);
>   /*
>    * no-bootscrub -> Free pages are not zeroed during boot.
>    */
> -static bool_t opt_bootscrub __initdata = 1;
> +static bool __read_mostly opt_bootscrub = true;
>   boolean_param("bootscrub", opt_bootscrub);
> +static bool __read_mostly bootscrub_done;
>   
>   /*
>    * bootscrub_chunk -> Amount of bytes to scrub lockstep on non-SMT CPUs
> @@ -1026,6 +1027,12 @@ static struct page_info *alloc_heap_pages(
>           }
>       }
>   
> +    if ( unlikely(opt_bootscrub && !bootscrub_done) )
> +    {
> +        for ( i = 0; i < (1U << order); i++ )
> +            scrub_one_page(&pg[i]);
> +    }
> +
>       if ( need_tlbflush )
>           filtered_flush_tlb_mask(tlbflush_timestamp);
>   
> @@ -1684,7 +1691,7 @@ unsigned int online_page(unsigned long mfn, uint32_t *status)
>       spin_unlock(&heap_lock);
>   
>       if ( (y & PGC_state) == PGC_state_offlined )
> -        free_heap_pages(pg, 0, false);
> +        free_heap_pages(pg, 0, true);
>   
>       return ret;
>   }
> @@ -1763,7 +1770,8 @@ static void init_heap_pages(
>               nr_pages -= n;
>           }
>   
> -        free_heap_pages(pg + i, 0, scrub_debug);
> +        free_heap_pages(pg + i, 0, scrub_debug ||
> +                                   (opt_bootscrub ? bootscrub_done : true));
>       }
>   }
>   
> @@ -2024,6 +2032,7 @@ static void __init scrub_heap_pages(void)
>           }
>       }
>   
> +    bootscrub_done = true;
>       printk("done.\n");
>   
>   #ifdef CONFIG_SCRUB_DEBUG
> @@ -2098,7 +2107,7 @@ void free_xenheap_pages(void *v, unsigned int order)
>   
>       memguard_guard_range(v, 1 << (order + PAGE_SHIFT));
>   
> -    free_heap_pages(virt_to_page(v), order, false);
> +    free_heap_pages(virt_to_page(v), order, true);
>   }
>   
>   #else
> @@ -2293,8 +2302,6 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
>       }
>       else
>       {
> -        bool_t scrub;
> -
>           if ( likely(d) && likely(d != dom_cow) )
>           {
>               /* NB. May recursively lock from relinquish_memory(). */
> @@ -2309,13 +2316,6 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
>               drop_dom_ref = !domain_adjust_tot_pages(d, -(1 << order));
>   
>               spin_unlock_recursive(&d->page_alloc_lock);
> -
> -            /*
> -             * Normally we expect a domain to clear pages before freeing them,
> -             * if it cares about the secrecy of their contents. However, after
> -             * a domain has died we assume responsibility for erasure.
> -             */
> -            scrub = d->is_dying || scrub_debug;
>           }
>           else
>           {
> @@ -2328,10 +2328,9 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
>                */
>               ASSERT(!d || !order);
>               drop_dom_ref = 0;
> -            scrub = 1;
>           }
>   
> -        free_heap_pages(pg, order, scrub);
> +        free_heap_pages(pg, order, true);
>       }
>   
>       if ( drop_dom_ref )
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm/page_alloc: always scrub pages given to the allocator

[Jan Beulich]

>>> On 01.10.18 at 11:58, <sergey.dyasli@citrix.com> wrote:
> Having the allocator return unscrubbed pages is a potential security
> concern: some domain can be given pages with memory contents of another
> domain. This may happen, for example, if a domain voluntarily releases
> its own memory (ballooning being the easiest way for doing this).

And we've always said that in this case it's the domain's responsibility
to scrub the memory of secrets it cares about. Therefore I'm at the
very least missing some background on this change of expectations.

> Change the allocator to always scrub the pages given to it by:
> 
> 1. free_xenheap_pages()
> 2. free_domheap_pages()
> 3. online_page()
> 4. init_heap_pages()
> 
> Performance testing has shown that on multi-node machines bootscrub
> vastly outperforms idle-loop scrubbing. So instead of marking all pages
> dirty initially, introduce bootscrub_done to track the completion of
> the process and eagerly scrub all allocated pages during boot.

I'm afraid I'm somewhat lost: There still is active boot time scrubbing,
or at least I can't see how that might be skipped (other than due to
"bootscrub=0"). I was actually expecting this to change at some
point. Am I perhaps simply mis-reading this part of the description?

> If bootscrub is disabled, then all pages will be marked as dirty right
> away and scrubbed either in idle-loop or eagerly during allocation.
> 
> After this patch, alloc_heap_pages() is guaranteed to return scrubbed
> pages to a caller unless MEMF_no_scrub flag was provided.

I also don't understand the point of this: Xen's internal allocations
have no need to come from scrubbed memory. This in particular
also puts under question the need to "eagerly scrub all allocated
pages during boot" (quoted from an earlier paragraph).

> @@ -1026,6 +1027,12 @@ static struct page_info *alloc_heap_pages(
>          }
>      }
>  
> +    if ( unlikely(opt_bootscrub && !bootscrub_done) )
> +    {
> +        for ( i = 0; i < (1U << order); i++ )
> +            scrub_one_page(&pg[i]);
> +    }

Do you really need to check two booleans in a case like this? Can't
bootscrub_done start out as true when opt_bootscrub is false?

Otherwise I think the use of unlikely() above is discouraged: It
generally shouldn't be used on && and || expressions, as the
annotation (on x86 at least) is meant to control the likelihood of
a particular conditional branch, yet && and || commonly require
multiple branches.

> @@ -1684,7 +1691,7 @@ unsigned int online_page(unsigned long mfn, uint32_t *status)
>      spin_unlock(&heap_lock);
>  
>      if ( (y & PGC_state) == PGC_state_offlined )
> -        free_heap_pages(pg, 0, false);
> +        free_heap_pages(pg, 0, true);

Are you really expecting secrets in freshly onlined memory?

> @@ -1763,7 +1770,8 @@ static void init_heap_pages(
>              nr_pages -= n;
>          }
>  
> -        free_heap_pages(pg + i, 0, scrub_debug);
> +        free_heap_pages(pg + i, 0, scrub_debug ||
> +                                   (opt_bootscrub ? bootscrub_done : true));

I think this would be easier to follow without conditional expression.

> @@ -2098,7 +2107,7 @@ void free_xenheap_pages(void *v, unsigned int order)
>  
>      memguard_guard_range(v, 1 << (order + PAGE_SHIFT));
>  
> -    free_heap_pages(virt_to_page(v), order, false);
> +    free_heap_pages(virt_to_page(v), order, true);
>  }
>  
>  #else

This sits in the "separate Xen heap" section - what use is scrubbing
here?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm/page_alloc: always scrub pages given to the allocator

[Andrew Cooper]

On 01/10/18 12:13, Jan Beulich wrote:
>>>> On 01.10.18 at 11:58, <sergey.dyasli@citrix.com> wrote:
>> Having the allocator return unscrubbed pages is a potential security
>> concern: some domain can be given pages with memory contents of another
>> domain. This may happen, for example, if a domain voluntarily releases
>> its own memory (ballooning being the easiest way for doing this).
> And we've always said that in this case it's the domain's responsibility
> to scrub the memory of secrets it cares about. Therefore I'm at the
> very least missing some background on this change of expectations.

You were on the call when this was discussed, along with the synchronous
scrubbing in destroydomain.

Put simply, the current behaviour is not good enough for a number of
security sensitive usecases.

The main reason however for doing this is the optimisations it enables,
and in particular, not double scrubbing most of our pages.

>
>> Change the allocator to always scrub the pages given to it by:
>>
>> 1. free_xenheap_pages()
>> 2. free_domheap_pages()
>> 3. online_page()
>> 4. init_heap_pages()
>>
>> Performance testing has shown that on multi-node machines bootscrub
>> vastly outperforms idle-loop scrubbing. So instead of marking all pages
>> dirty initially, introduce bootscrub_done to track the completion of
>> the process and eagerly scrub all allocated pages during boot.
> I'm afraid I'm somewhat lost: There still is active boot time scrubbing,
> or at least I can't see how that might be skipped (other than due to
> "bootscrub=0"). I was actually expecting this to change at some
> point. Am I perhaps simply mis-reading this part of the description?

No.  Sergey tried that, and found a massive perf difference between
scrubbing in the idle loop and scrubbing at boot.  (1.2s vs 40s iirc)

Scrubbing at boot has some deliberate optimisations to reduce the
pressure on the heap lock, and I expect that is where the performance
difference lies.  It is an issue which wants looking into irrespective
of other changes.

>
>> If bootscrub is disabled, then all pages will be marked as dirty right
>> away and scrubbed either in idle-loop or eagerly during allocation.
>>
>> After this patch, alloc_heap_pages() is guaranteed to return scrubbed
>> pages to a caller unless MEMF_no_scrub flag was provided.
> I also don't understand the point of this: Xen's internal allocations
> have no need to come from scrubbed memory.

This isn't true.  Almost every caller re-zeroes an allocated page which
is the cause of the double scrubbing in most cases.

By having the allocators guarantee to hand out zeroed pages, we can
avoid the redundant scrubbing.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm/page_alloc: always scrub pages given to the allocator

[George Dunlap]

On 10/01/2018 02:12 PM, Andrew Cooper wrote:
> On 01/10/18 12:13, Jan Beulich wrote:
>>>>> On 01.10.18 at 11:58, <sergey.dyasli@citrix.com> wrote:
>>> Having the allocator return unscrubbed pages is a potential security
>>> concern: some domain can be given pages with memory contents of another
>>> domain. This may happen, for example, if a domain voluntarily releases
>>> its own memory (ballooning being the easiest way for doing this).
>> And we've always said that in this case it's the domain's responsibility
>> to scrub the memory of secrets it cares about. Therefore I'm at the
>> very least missing some background on this change of expectations.
> 
> You were on the call when this was discussed, along with the synchronous
> scrubbing in destroydomain.
> 
> Put simply, the current behaviour is not good enough for a number of
> security sensitive usecases.
> 
> The main reason however for doing this is the optimisations it enables,
> and in particular, not double scrubbing most of our pages.

All of that should have been in the changelog, at least in summary form,
regardless of where else it may have been discussed.

 -George


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm/page_alloc: always scrub pages given to the allocator

[Jan Beulich]

>>> On 01.10.18 at 15:12, <andrew.cooper3@citrix.com> wrote:
> On 01/10/18 12:13, Jan Beulich wrote:
>>>>> On 01.10.18 at 11:58, <sergey.dyasli@citrix.com> wrote:
>>> Having the allocator return unscrubbed pages is a potential security
>>> concern: some domain can be given pages with memory contents of another
>>> domain. This may happen, for example, if a domain voluntarily releases
>>> its own memory (ballooning being the easiest way for doing this).
>> And we've always said that in this case it's the domain's responsibility
>> to scrub the memory of secrets it cares about. Therefore I'm at the
>> very least missing some background on this change of expectations.
> 
> You were on the call when this was discussed, along with the synchronous
> scrubbing in destroydomain.

Quite possible, but it has been a while.

> Put simply, the current behaviour is not good enough for a number of
> security sensitive usecases.

Well, I'm looking forward for Sergey to expand on this in the commit
message.

> The main reason however for doing this is the optimisations it enables,
> and in particular, not double scrubbing most of our pages.

Well, wait - scrubbing != zeroing (taking into account also what you
say further down).

>>> Change the allocator to always scrub the pages given to it by:
>>>
>>> 1. free_xenheap_pages()
>>> 2. free_domheap_pages()
>>> 3. online_page()
>>> 4. init_heap_pages()
>>>
>>> Performance testing has shown that on multi-node machines bootscrub
>>> vastly outperforms idle-loop scrubbing. So instead of marking all pages
>>> dirty initially, introduce bootscrub_done to track the completion of
>>> the process and eagerly scrub all allocated pages during boot.
>> I'm afraid I'm somewhat lost: There still is active boot time scrubbing,
>> or at least I can't see how that might be skipped (other than due to
>> "bootscrub=0"). I was actually expecting this to change at some
>> point. Am I perhaps simply mis-reading this part of the description?
> 
> No.  Sergey tried that, and found a massive perf difference between
> scrubbing in the idle loop and scrubbing at boot.  (1.2s vs 40s iirc)

That's not something you can reasonably compare, imo: For one,
it is certainly expected for the background scrubbing to be slower,
simply because of other activity on the system. And then 1.2s
looks awfully small for a multi-Tb system. Yet it is mainly large
systems where the synchronous boot time scrubbing is a problem.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm/page_alloc: always scrub pages given to the allocator

[Sergey Dyasli]

On Mon, 2018-10-01 at 07:38 -0600, Jan Beulich wrote:
> > > > On 01.10.18 at 15:12, <andrew.cooper3@citrix.com> wrote:
> > 
> > On 01/10/18 12:13, Jan Beulich wrote:
> > > > > > On 01.10.18 at 11:58, <sergey.dyasli@citrix.com> wrote:
> > > > 
> > > > Having the allocator return unscrubbed pages is a potential security
> > > > concern: some domain can be given pages with memory contents of another
> > > > domain. This may happen, for example, if a domain voluntarily releases
> > > > its own memory (ballooning being the easiest way for doing this).
> > > 
> > > And we've always said that in this case it's the domain's responsibility
> > > to scrub the memory of secrets it cares about. Therefore I'm at the
> > > very least missing some background on this change of expectations.
> > 
> > You were on the call when this was discussed, along with the synchronous
> > scrubbing in destroydomain.
> 
> Quite possible, but it has been a while.
> 
> > Put simply, the current behaviour is not good enough for a number of
> > security sensitive usecases.
> 
> Well, I'm looking forward for Sergey to expand on this in the commit
> message.
> 
> > The main reason however for doing this is the optimisations it enables,
> > and in particular, not double scrubbing most of our pages.
> 
> Well, wait - scrubbing != zeroing (taking into account also what you
> say further down).
> 
> > > > Change the allocator to always scrub the pages given to it by:
> > > > 
> > > > 1. free_xenheap_pages()
> > > > 2. free_domheap_pages()
> > > > 3. online_page()
> > > > 4. init_heap_pages()
> > > > 
> > > > Performance testing has shown that on multi-node machines bootscrub
> > > > vastly outperforms idle-loop scrubbing. So instead of marking all pages
> > > > dirty initially, introduce bootscrub_done to track the completion of
> > > > the process and eagerly scrub all allocated pages during boot.
> > > 
> > > I'm afraid I'm somewhat lost: There still is active boot time scrubbing,
> > > or at least I can't see how that might be skipped (other than due to
> > > "bootscrub=0"). I was actually expecting this to change at some
> > > point. Am I perhaps simply mis-reading this part of the description?
> > 
> > No.  Sergey tried that, and found a massive perf difference between
> > scrubbing in the idle loop and scrubbing at boot.  (1.2s vs 40s iirc)
> 
> That's not something you can reasonably compare, imo: For one,
> it is certainly expected for the background scrubbing to be slower,
> simply because of other activity on the system. And then 1.2s
> looks awfully small for a multi-Tb system. Yet it is mainly large
> systems where the synchronous boot time scrubbing is a problem.

Let me throw in some numbers.

Performance of current idle loop scrubbing is just not good enough:
on 8 nodes, 32 CPUs and 512GB RAM machine it takes ~40 seconds to scrub
all the memory instead of ~8 seconds for current bootscrub implementation.

This was measured while synchronously waiting for CPUs to scrub all the
memory in idle-loop. But scrubbing can happen in background, of course.

-- 
Thanks,
Sergey
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm/page_alloc: always scrub pages given to the allocator

[Boris Ostrovsky]

On 10/1/18 9:12 AM, Andrew Cooper wrote:
> On 01/10/18 12:13, Jan Beulich wrote:
>>>>> On 01.10.18 at 11:58, <sergey.dyasli@citrix.com> wrote:
>>> Having the allocator return unscrubbed pages is a potential security
>>> concern: some domain can be given pages with memory contents of another
>>> domain. This may happen, for example, if a domain voluntarily releases
>>> its own memory (ballooning being the easiest way for doing this).
>> And we've always said that in this case it's the domain's responsibility
>> to scrub the memory of secrets it cares about. Therefore I'm at the
>> very least missing some background on this change of expectations.
> You were on the call when this was discussed, along with the synchronous
> scrubbing in destroydomain.
>
> Put simply, the current behaviour is not good enough for a number of
> security sensitive usecases.
>
> The main reason however for doing this is the optimisations it enables,
> and in particular, not double scrubbing most of our pages.

OTOH, it introduces double scrubbing for ballooning because (at least
Linux) guests do scrub memory before handing it back to the hypervisor.

-boris


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm/page_alloc: always scrub pages given to the allocator

[George Dunlap]

On 10/01/2018 02:44 PM, Boris Ostrovsky wrote:
> On 10/1/18 9:12 AM, Andrew Cooper wrote:
>> On 01/10/18 12:13, Jan Beulich wrote:
>>>>>> On 01.10.18 at 11:58, <sergey.dyasli@citrix.com> wrote:
>>>> Having the allocator return unscrubbed pages is a potential security
>>>> concern: some domain can be given pages with memory contents of another
>>>> domain. This may happen, for example, if a domain voluntarily releases
>>>> its own memory (ballooning being the easiest way for doing this).
>>> And we've always said that in this case it's the domain's responsibility
>>> to scrub the memory of secrets it cares about. Therefore I'm at the
>>> very least missing some background on this change of expectations.
>> You were on the call when this was discussed, along with the synchronous
>> scrubbing in destroydomain.
>>
>> Put simply, the current behaviour is not good enough for a number of
>> security sensitive usecases.
>>
>> The main reason however for doing this is the optimisations it enables,
>> and in particular, not double scrubbing most of our pages.
> 
> OTOH, it introduces double scrubbing for ballooning because (at least
> Linux) guests do scrub memory before handing it back to the hypervisor.

We could add a Xen feature flag which tells the guest balloon driver
whether the hypervisor will scrub pages (and thus it doesn't need to).

Andy, wasn't unconditional scrubbing of guest pages also required by
some certification or other?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm/page_alloc: always scrub pages given to the allocator

[George Dunlap]

On 10/01/2018 02:44 PM, Sergey Dyasli wrote:
> On Mon, 2018-10-01 at 07:38 -0600, Jan Beulich wrote:
>>>>> On 01.10.18 at 15:12, <andrew.cooper3@citrix.com> wrote:
>>>
>>> On 01/10/18 12:13, Jan Beulich wrote:
>>>>>>> On 01.10.18 at 11:58, <sergey.dyasli@citrix.com> wrote:
>>>>>
>>>>> Having the allocator return unscrubbed pages is a potential security
>>>>> concern: some domain can be given pages with memory contents of another
>>>>> domain. This may happen, for example, if a domain voluntarily releases
>>>>> its own memory (ballooning being the easiest way for doing this).
>>>>
>>>> And we've always said that in this case it's the domain's responsibility
>>>> to scrub the memory of secrets it cares about. Therefore I'm at the
>>>> very least missing some background on this change of expectations.
>>>
>>> You were on the call when this was discussed, along with the synchronous
>>> scrubbing in destroydomain.
>>
>> Quite possible, but it has been a while.
>>
>>> Put simply, the current behaviour is not good enough for a number of
>>> security sensitive usecases.
>>
>> Well, I'm looking forward for Sergey to expand on this in the commit
>> message.
>>
>>> The main reason however for doing this is the optimisations it enables,
>>> and in particular, not double scrubbing most of our pages.
>>
>> Well, wait - scrubbing != zeroing (taking into account also what you
>> say further down).
>>
>>>>> Change the allocator to always scrub the pages given to it by:
>>>>>
>>>>> 1. free_xenheap_pages()
>>>>> 2. free_domheap_pages()
>>>>> 3. online_page()
>>>>> 4. init_heap_pages()
>>>>>
>>>>> Performance testing has shown that on multi-node machines bootscrub
>>>>> vastly outperforms idle-loop scrubbing. So instead of marking all pages
>>>>> dirty initially, introduce bootscrub_done to track the completion of
>>>>> the process and eagerly scrub all allocated pages during boot.
>>>>
>>>> I'm afraid I'm somewhat lost: There still is active boot time scrubbing,
>>>> or at least I can't see how that might be skipped (other than due to
>>>> "bootscrub=0"). I was actually expecting this to change at some
>>>> point. Am I perhaps simply mis-reading this part of the description?
>>>
>>> No.  Sergey tried that, and found a massive perf difference between
>>> scrubbing in the idle loop and scrubbing at boot.  (1.2s vs 40s iirc)
>>
>> That's not something you can reasonably compare, imo: For one,
>> it is certainly expected for the background scrubbing to be slower,
>> simply because of other activity on the system. And then 1.2s
>> looks awfully small for a multi-Tb system. Yet it is mainly large
>> systems where the synchronous boot time scrubbing is a problem.
> 
> Let me throw in some numbers.
> 
> Performance of current idle loop scrubbing is just not good enough:
> on 8 nodes, 32 CPUs and 512GB RAM machine it takes ~40 seconds to scrub
> all the memory instead of ~8 seconds for current bootscrub implementation.
> 
> This was measured while synchronously waiting for CPUs to scrub all the
> memory in idle-loop. But scrubbing can happen in background, of course.

Right, the whole point of idle loop scrubbing is that you *don't*
syncronously wait for *all* the memory to finish scrubbing before you
can use part of it.  So why is this an issue for you guys -- what
concrete problem did it cause, that the full amount of memory took 40s
to finish scrubbing rather than only 8s?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm/page_alloc: always scrub pages given to the allocator

[Andrew Cooper]

On 01/10/18 14:50, George Dunlap wrote:
> On 10/01/2018 02:44 PM, Boris Ostrovsky wrote:
>> On 10/1/18 9:12 AM, Andrew Cooper wrote:
>>> On 01/10/18 12:13, Jan Beulich wrote:
>>>>>>> On 01.10.18 at 11:58, <sergey.dyasli@citrix.com> wrote:
>>>>> Having the allocator return unscrubbed pages is a potential security
>>>>> concern: some domain can be given pages with memory contents of another
>>>>> domain. This may happen, for example, if a domain voluntarily releases
>>>>> its own memory (ballooning being the easiest way for doing this).
>>>> And we've always said that in this case it's the domain's responsibility
>>>> to scrub the memory of secrets it cares about. Therefore I'm at the
>>>> very least missing some background on this change of expectations.
>>> You were on the call when this was discussed, along with the synchronous
>>> scrubbing in destroydomain.
>>>
>>> Put simply, the current behaviour is not good enough for a number of
>>> security sensitive usecases.
>>>
>>> The main reason however for doing this is the optimisations it enables,
>>> and in particular, not double scrubbing most of our pages.
>> OTOH, it introduces double scrubbing for ballooning because (at least
>> Linux) guests do scrub memory before handing it back to the hypervisor.
> We could add a Xen feature flag which tells the guest balloon driver
> whether the hypervisor will scrub pages (and thus it doesn't need to).
>
> Andy, wasn't unconditional scrubbing of guest pages also required by
> some certification or other?

It was for Common Criteria, and not trusting guests to zero their own
pages is by far the simplest way of ensuring that they can't set up a
sidechannel.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm/page_alloc: always scrub pages given to the allocator

[Boris Ostrovsky]

On 10/1/18 9:50 AM, George Dunlap wrote:
> On 10/01/2018 02:44 PM, Boris Ostrovsky wrote:
>> On 10/1/18 9:12 AM, Andrew Cooper wrote:
>>> On 01/10/18 12:13, Jan Beulich wrote:
>>>>>>> On 01.10.18 at 11:58, <sergey.dyasli@citrix.com> wrote:
>>>>> Having the allocator return unscrubbed pages is a potential security
>>>>> concern: some domain can be given pages with memory contents of another
>>>>> domain. This may happen, for example, if a domain voluntarily releases
>>>>> its own memory (ballooning being the easiest way for doing this).
>>>> And we've always said that in this case it's the domain's responsibility
>>>> to scrub the memory of secrets it cares about. Therefore I'm at the
>>>> very least missing some background on this change of expectations.
>>> You were on the call when this was discussed, along with the synchronous
>>> scrubbing in destroydomain.
>>>
>>> Put simply, the current behaviour is not good enough for a number of
>>> security sensitive usecases.
>>>
>>> The main reason however for doing this is the optimisations it enables,
>>> and in particular, not double scrubbing most of our pages.
>> OTOH, it introduces double scrubbing for ballooning because (at least
>> Linux) guests do scrub memory before handing it back to the hypervisor.
> We could add a Xen feature flag which tells the guest balloon driver
> whether the hypervisor will scrub pages (and thus it doesn't need to).

We can, but we are regressing existing guests.

Can we except ballooned pages from being scrubbed, and instead have a
guest request scrubbing if it has concerns (that Andrew mentioned) about
this?

-boris


>
> Andy, wasn't unconditional scrubbing of guest pages also required by
> some certification or other?
>
>  -George


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm/page_alloc: always scrub pages given to the allocator

[Sergey Dyasli]

On Mon, 2018-10-01 at 07:38 -0600, Jan Beulich wrote:
> > > > On 01.10.18 at 15:12, <andrew.cooper3@citrix.com> wrote:
> > 
> > On 01/10/18 12:13, Jan Beulich wrote:
> > > > > > On 01.10.18 at 11:58, <sergey.dyasli@citrix.com> wrote:
> > > > 
> > > > Having the allocator return unscrubbed pages is a potential security
> > > > concern: some domain can be given pages with memory contents of another
> > > > domain. This may happen, for example, if a domain voluntarily releases
> > > > its own memory (ballooning being the easiest way for doing this).
> > > 
> > > And we've always said that in this case it's the domain's responsibility
> > > to scrub the memory of secrets it cares about. Therefore I'm at the
> > > very least missing some background on this change of expectations.
> > 
> > You were on the call when this was discussed, along with the synchronous
> > scrubbing in destroydomain.
> 
> Quite possible, but it has been a while.
> 
> > Put simply, the current behaviour is not good enough for a number of
> > security sensitive usecases.
> 
> Well, I'm looking forward for Sergey to expand on this in the commit
> message.

Jan,

I think this is the main argument here: what to do about those security
sensitive use cases? Scrubbing everything unconditionally might be a too
radical approach. Would inroducing a new cmdline param be appropriate?

> 
> > The main reason however for doing this is the optimisations it enables,
> > and in particular, not double scrubbing most of our pages.
> 
> Well, wait - scrubbing != zeroing (taking into account also what you
> say further down).

Andrew,

I'm not yet convinced myself about the value that returning always zeroed
pages from the allocator provides. Most of the pages are given to guests
anyway, and re-zeroing a few pages in the hypervisor doesn't sound
too bad. But maybe I'm just not aware of cases where Xen performs large
allocations and zeroes them afterwards?

-- 
Thanks,
Sergey
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm/page_alloc: always scrub pages given to the allocator

[Sergey Dyasli]

On Mon, 2018-10-01 at 14:54 +0100, George Dunlap wrote:
> On 10/01/2018 02:44 PM, Sergey Dyasli wrote:
> > On Mon, 2018-10-01 at 07:38 -0600, Jan Beulich wrote:
> > > > > > On 01.10.18 at 15:12, <andrew.cooper3@citrix.com> wrote:
> > > > 
> > > > On 01/10/18 12:13, Jan Beulich wrote:
> > > > > > > > On 01.10.18 at 11:58, <sergey.dyasli@citrix.com> wrote:
> > > > > > 
> > > > > > Having the allocator return unscrubbed pages is a potential security
> > > > > > concern: some domain can be given pages with memory contents of another
> > > > > > domain. This may happen, for example, if a domain voluntarily releases
> > > > > > its own memory (ballooning being the easiest way for doing this).
> > > > > 
> > > > > And we've always said that in this case it's the domain's responsibility
> > > > > to scrub the memory of secrets it cares about. Therefore I'm at the
> > > > > very least missing some background on this change of expectations.
> > > > 
> > > > You were on the call when this was discussed, along with the synchronous
> > > > scrubbing in destroydomain.
> > > 
> > > Quite possible, but it has been a while.
> > > 
> > > > Put simply, the current behaviour is not good enough for a number of
> > > > security sensitive usecases.
> > > 
> > > Well, I'm looking forward for Sergey to expand on this in the commit
> > > message.
> > > 
> > > > The main reason however for doing this is the optimisations it enables,
> > > > and in particular, not double scrubbing most of our pages.
> > > 
> > > Well, wait - scrubbing != zeroing (taking into account also what you
> > > say further down).
> > > 
> > > > > > Change the allocator to always scrub the pages given to it by:
> > > > > > 
> > > > > > 1. free_xenheap_pages()
> > > > > > 2. free_domheap_pages()
> > > > > > 3. online_page()
> > > > > > 4. init_heap_pages()
> > > > > > 
> > > > > > Performance testing has shown that on multi-node machines bootscrub
> > > > > > vastly outperforms idle-loop scrubbing. So instead of marking all pages
> > > > > > dirty initially, introduce bootscrub_done to track the completion of
> > > > > > the process and eagerly scrub all allocated pages during boot.
> > > > > 
> > > > > I'm afraid I'm somewhat lost: There still is active boot time scrubbing,
> > > > > or at least I can't see how that might be skipped (other than due to
> > > > > "bootscrub=0"). I was actually expecting this to change at some
> > > > > point. Am I perhaps simply mis-reading this part of the description?
> > > > 
> > > > No.  Sergey tried that, and found a massive perf difference between
> > > > scrubbing in the idle loop and scrubbing at boot.  (1.2s vs 40s iirc)
> > > 
> > > That's not something you can reasonably compare, imo: For one,
> > > it is certainly expected for the background scrubbing to be slower,
> > > simply because of other activity on the system. And then 1.2s
> > > looks awfully small for a multi-Tb system. Yet it is mainly large
> > > systems where the synchronous boot time scrubbing is a problem.
> > 
> > Let me throw in some numbers.
> > 
> > Performance of current idle loop scrubbing is just not good enough:
> > on 8 nodes, 32 CPUs and 512GB RAM machine it takes ~40 seconds to scrub
> > all the memory instead of ~8 seconds for current bootscrub implementation.
> > 
> > This was measured while synchronously waiting for CPUs to scrub all the
> > memory in idle-loop. But scrubbing can happen in background, of course.
> 
> Right, the whole point of idle loop scrubbing is that you *don't*
> syncronously wait for *all* the memory to finish scrubbing before you
> can use part of it.  So why is this an issue for you guys -- what
> concrete problem did it cause, that the full amount of memory took 40s
> to finish scrubbing rather than only 8s?

There is no issue at the moment. Using idle loop to scrub all the memory
is just not viable: it doesn't scale. How long does it currently take
to bootscrub a multi-Tb system? If it takes a few minutes then I fear
that it might take an hour to idle-scrub it.

-- 
Thanks,
Sergey
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm/page_alloc: always scrub pages given to the allocator

[Sergey Dyasli]

On 01/10/18 12:13, Jan Beulich wrote:
>>>> On 01.10.18 at 11:58, <sergey.dyasli@citrix.com> wrote:
>> Having the allocator return unscrubbed pages is a potential security
>> concern: some domain can be given pages with memory contents of another
>> domain. This may happen, for example, if a domain voluntarily releases
>> its own memory (ballooning being the easiest way for doing this).
> 
> And we've always said that in this case it's the domain's responsibility
> to scrub the memory of secrets it cares about. Therefore I'm at the
> very least missing some background on this change of expectations.
> 
>> Change the allocator to always scrub the pages given to it by:
>>
>> 1. free_xenheap_pages()
>> 2. free_domheap_pages()
>> 3. online_page()
>> 4. init_heap_pages()
>>
>> Performance testing has shown that on multi-node machines bootscrub
>> vastly outperforms idle-loop scrubbing. So instead of marking all pages
>> dirty initially, introduce bootscrub_done to track the completion of
>> the process and eagerly scrub all allocated pages during boot.
> 
> I'm afraid I'm somewhat lost: There still is active boot time scrubbing,
> or at least I can't see how that might be skipped (other than due to
> "bootscrub=0"). I was actually expecting this to change at some
> point. Am I perhaps simply mis-reading this part of the description?
> 
>> If bootscrub is disabled, then all pages will be marked as dirty right
>> away and scrubbed either in idle-loop or eagerly during allocation.
>>
>> After this patch, alloc_heap_pages() is guaranteed to return scrubbed
>> pages to a caller unless MEMF_no_scrub flag was provided.
> 
> I also don't understand the point of this: Xen's internal allocations
> have no need to come from scrubbed memory. This in particular
> also puts under question the need to "eagerly scrub all allocated
> pages during boot" (quoted from an earlier paragraph).

There are ways to share a Xen's page with a guest. So from a security
point of view, there is no guarantee that a page allocated with
alloc_xenheap_pages() will not end up accessible by some guest.

--
Thanks,
Sergey

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm/page_alloc: always scrub pages given to the allocator

[Andrew Cooper]

On 01/10/18 14:57, Boris Ostrovsky wrote:
> On 10/1/18 9:50 AM, George Dunlap wrote:
>> On 10/01/2018 02:44 PM, Boris Ostrovsky wrote:
>>> On 10/1/18 9:12 AM, Andrew Cooper wrote:
>>>> On 01/10/18 12:13, Jan Beulich wrote:
>>>>>>>> On 01.10.18 at 11:58, <sergey.dyasli@citrix.com> wrote:
>>>>>> Having the allocator return unscrubbed pages is a potential security
>>>>>> concern: some domain can be given pages with memory contents of another
>>>>>> domain. This may happen, for example, if a domain voluntarily releases
>>>>>> its own memory (ballooning being the easiest way for doing this).
>>>>> And we've always said that in this case it's the domain's responsibility
>>>>> to scrub the memory of secrets it cares about. Therefore I'm at the
>>>>> very least missing some background on this change of expectations.
>>>> You were on the call when this was discussed, along with the synchronous
>>>> scrubbing in destroydomain.
>>>>
>>>> Put simply, the current behaviour is not good enough for a number of
>>>> security sensitive usecases.
>>>>
>>>> The main reason however for doing this is the optimisations it enables,
>>>> and in particular, not double scrubbing most of our pages.
>>> OTOH, it introduces double scrubbing for ballooning because (at least
>>> Linux) guests do scrub memory before handing it back to the hypervisor.
>> We could add a Xen feature flag which tells the guest balloon driver
>> whether the hypervisor will scrub pages (and thus it doesn't need to).
> We can, but we are regressing existing guests.

There is no regression.  How Xen handles its free memory is unrelated to
how the guest handles its free memory, and real usecases exist (AMD SEV
most obviously) where there deliberately isn't full trust between the
guest and the hypervisor.

It is suboptimal in the case where Xen and the guest mutually trust each
other, but that isn't the only case which exists.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm/page_alloc: always scrub pages given to the allocator

[Jan Beulich]

>>> On 01.10.18 at 16:11, <sergey.dyasli@citrix.com> wrote:
> I think this is the main argument here: what to do about those security
> sensitive use cases? Scrubbing everything unconditionally might be a too
> radical approach. Would inroducing a new cmdline param be appropriate?

Yes, I'm surely fine with this being an optional (and hence command
line driven) mode.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm/page_alloc: always scrub pages given to the allocator

[Jan Beulich]

>>> On 01.10.18 at 16:40, <sergey.dyasli@citrix.com> wrote:
> On 01/10/18 12:13, Jan Beulich wrote:
>>>>> On 01.10.18 at 11:58, <sergey.dyasli@citrix.com> wrote:
>>> After this patch, alloc_heap_pages() is guaranteed to return scrubbed
>>> pages to a caller unless MEMF_no_scrub flag was provided.
>> 
>> I also don't understand the point of this: Xen's internal allocations
>> have no need to come from scrubbed memory. This in particular
>> also puts under question the need to "eagerly scrub all allocated
>> pages during boot" (quoted from an earlier paragraph).
> 
> There are ways to share a Xen's page with a guest. So from a security
> point of view, there is no guarantee that a page allocated with
> alloc_xenheap_pages() will not end up accessible by some guest.

But this is the exception, not the rule, and hence the code enabling
the sharing is responsible for initializing the page suitably.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm/page_alloc: always scrub pages given to the allocator

[Jan Beulich]

>>> On 01.10.18 at 16:28, <sergey.dyasli@citrix.com> wrote:
> On Mon, 2018-10-01 at 14:54 +0100, George Dunlap wrote:
>> Right, the whole point of idle loop scrubbing is that you *don't*
>> syncronously wait for *all* the memory to finish scrubbing before you
>> can use part of it.  So why is this an issue for you guys -- what
>> concrete problem did it cause, that the full amount of memory took 40s
>> to finish scrubbing rather than only 8s?
> 
> There is no issue at the moment. Using idle loop to scrub all the memory
> is just not viable: it doesn't scale. How long does it currently take
> to bootscrub a multi-Tb system? If it takes a few minutes then I fear
> that it might take an hour to idle-scrub it.

But that's fine, at least in theory, as long as no scrubbing will be
skipped when it was actually needed. The system will be slightly
more busy until done with the scrubbing, but that's exactly the
expected price to pay for the cut down on boot time.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel