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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.10.18 at 11:58, <sergey.dyasli@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel