[v2] mm: option to _always_ scrub freed domheap pages

Commit Message

Eslam Elnikety May 7, 2019, 11:34 a.m. UTC

Give the administrator further control on when to scrub domheap pages by adding
an option to always scrub. This is a safety feature that, when enabled,
prevents a (buggy) domain from leaking secrets if it accidentally frees a page
without proper scrubbing.

Signed-off-by: Eslam Elnikety <[e-mail]>
Acked-by: George Dunlap <[e-mail]>
---
    Changes in v2:
        - Renamed parameter to scrub-domheap, and now at the right place
        - Used "bool __read_mostly", no zero init, and correct comment style
        - Added George's A-b
---
 docs/misc/xen-command-line.pandoc | 8 ++++++++
 xen/common/page_alloc.c           | 9 +++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

Comments

Jan Beulich May 7, 2019, 12:11 p.m. UTC | #1

>>> On 07.05.19 at 13:34, <[e-mail]> wrote:
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -214,6 +214,10 @@ custom_param("bootscrub", parse_bootscrub_param);
>  static unsigned long __initdata opt_bootscrub_chunk = MB(128);
>  size_param("bootscrub_chunk", opt_bootscrub_chunk);
>  
> + /* scrub-domheap -> Domheap pages are scrubbed when freed */
> +static bool __read_mostly opt_scrub_domheap;
> +boolean_param("scrub-domheap", opt_scrub_domheap);

Upon 2nd thought this, btw, would seem to be an excellent candidate
for becoming a runtime parameter.

> @@ -2378,9 +2382,10 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
>              /*
>               * 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.
> +             * a domain has died we assume responsibility for erasure. We do
> +             * scrub regardless if option scrub_domheap is set.
>               */
> -            scrub = d->is_dying || scrub_debug;
> +            scrub = d->is_dying || scrub_debug || opt_scrub_domheap;

Did you consider setting opt_scrub_domheap when scrub_debug is
set? This would shorten the (runtime) calculation here by a tiny bit,
at the price of doing one more thing once while booting.

Jan

George Dunlap May 7, 2019, 1:15 p.m. UTC | #2

On 5/7/19 1:11 PM, Jan Beulich wrote:
>>>> On 07.05.19 at 13:34, <[e-mail]> wrote:
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -214,6 +214,10 @@ custom_param("bootscrub", parse_bootscrub_param);
>>  static unsigned long __initdata opt_bootscrub_chunk = MB(128);
>>  size_param("bootscrub_chunk", opt_bootscrub_chunk);
>>  
>> + /* scrub-domheap -> Domheap pages are scrubbed when freed */
>> +static bool __read_mostly opt_scrub_domheap;
>> +boolean_param("scrub-domheap", opt_scrub_domheap);
> 
> Upon 2nd thought this, btw, would seem to be an excellent candidate
> for becoming a runtime parameter.
> 
>> @@ -2378,9 +2382,10 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
>>              /*
>>               * 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.
>> +             * a domain has died we assume responsibility for erasure. We do
>> +             * scrub regardless if option scrub_domheap is set.
>>               */
>> -            scrub = d->is_dying || scrub_debug;
>> +            scrub = d->is_dying || scrub_debug || opt_scrub_domheap;
> 
> Did you consider setting opt_scrub_domheap when scrub_debug is
> set? This would shorten the (runtime) calculation here by a tiny bit,
> at the price of doing one more thing once while booting.

Just for clarification Jan -- did you mean, "I'm happy for this to go in
as it is, but if you feel like it, here are two improvements"?

 -George

Jan Beulich May 7, 2019, 1:55 p.m. UTC | #3

>>> On 07.05.19 at 15:15, <[e-mail]> wrote:
> On 5/7/19 1:11 PM, Jan Beulich wrote:
>>>>> On 07.05.19 at 13:34, <[e-mail]> wrote:
>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -214,6 +214,10 @@ custom_param("bootscrub", parse_bootscrub_param);
>>>  static unsigned long __initdata opt_bootscrub_chunk = MB(128);
>>>  size_param("bootscrub_chunk", opt_bootscrub_chunk);
>>>  
>>> + /* scrub-domheap -> Domheap pages are scrubbed when freed */
>>> +static bool __read_mostly opt_scrub_domheap;
>>> +boolean_param("scrub-domheap", opt_scrub_domheap);
>> 
>> Upon 2nd thought this, btw, would seem to be an excellent candidate
>> for becoming a runtime parameter.
>> 
>>> @@ -2378,9 +2382,10 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
>>>              /*
>>>               * 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.
>>> +             * a domain has died we assume responsibility for erasure. We do
>>> +             * scrub regardless if option scrub_domheap is set.
>>>               */
>>> -            scrub = d->is_dying || scrub_debug;
>>> +            scrub = d->is_dying || scrub_debug || opt_scrub_domheap;
>> 
>> Did you consider setting opt_scrub_domheap when scrub_debug is
>> set? This would shorten the (runtime) calculation here by a tiny bit,
>> at the price of doing one more thing once while booting.
> 
> Just for clarification Jan -- did you mean, "I'm happy for this to go in
> as it is, but if you feel like it, here are two improvements"?

Yes (maybe "I'd prefer" to replace "if you feel like it").

Jan

Eslam Elnikety May 7, 2019, 11:37 p.m. UTC | #4

> On 7. May 2019, at 14:11, Jan Beulich <[e-mail]> wrote:
> 
>>>> On 07.05.19 at 13:34, <[e-mail]> wrote:
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -214,6 +214,10 @@ custom_param("bootscrub", parse_bootscrub_param);
>> static unsigned long __initdata opt_bootscrub_chunk = MB(128);
>> size_param("bootscrub_chunk", opt_bootscrub_chunk);
>> 
>> + /* scrub-domheap -> Domheap pages are scrubbed when freed */
>> +static bool __read_mostly opt_scrub_domheap;
>> +boolean_param("scrub-domheap", opt_scrub_domheap);
> 
> Upon 2nd thought this, btw, would seem to be an excellent candidate
> for becoming a runtime parameter.

True.

> 
>> @@ -2378,9 +2382,10 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
>>             /*
>>              * 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.
>> +             * a domain has died we assume responsibility for erasure. We do
>> +             * scrub regardless if option scrub_domheap is set.
>>              */
>> -            scrub = d->is_dying || scrub_debug;
>> +            scrub = d->is_dying || scrub_debug || opt_scrub_domheap;
> 
> Did you consider setting opt_scrub_domheap when scrub_debug is
> set? This would shorten the (runtime) calculation here by a tiny bit,
> at the price of doing one more thing once while booting.

Interesting. I have not particularly thought about that. Granted; this would shorten the “scrub” bool calculation. One would probably define a bool ‘always_scrub’ that gets set at boot ‘always_scrub = scrub_debug || opt_scrub_domheap’, and use that new bool in the hunk at hand here. (Having opt_scrub_domheap as a runtime parameter and re-evaluating always_scrub should not be much of a complication either). 

In any case, given your response to George earlier, I would rather decouple these improvements from this patch. I would be happy to re-work these improvements at a later point if the community feels strongly about them.

> 
> Jan
> 
>

Patch

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 6db82f302e..771333fc8a 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1779,6 +1779,14 @@  sockets, &c.  This will reduce performance somewhat, particularly on
 systems with hyperthreading enabled, but should reduce power by
 enabling more sockets and cores to go into deeper sleep states.
 
+### scrub-domheap
+> `= <boolean>`
+
+> Default: `false`
+
+Scrub domains' freed pages. This is a safety net against a (buggy) domain
+accidentally leaking secrets by releasing pages without proper sanitization.
+
 ### serial_tx_buffer
 > `= <size>`
 
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index be44158033..9c12d71fc1 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -214,6 +214,10 @@  custom_param("bootscrub", parse_bootscrub_param);
 static unsigned long __initdata opt_bootscrub_chunk = MB(128);
 size_param("bootscrub_chunk", opt_bootscrub_chunk);
 
+ /* scrub-domheap -> Domheap pages are scrubbed when freed */
+static bool __read_mostly opt_scrub_domheap;
+boolean_param("scrub-domheap", opt_scrub_domheap);
+
 #ifdef CONFIG_SCRUB_DEBUG
 static bool __read_mostly scrub_debug;
 #else
@@ -2378,9 +2382,10 @@  void free_domheap_pages(struct page_info *pg, unsigned int order)
             /*
              * 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.
+             * a domain has died we assume responsibility for erasure. We do
+             * scrub regardless if option scrub_domheap is set.
              */
-            scrub = d->is_dying || scrub_debug;
+            scrub = d->is_dying || scrub_debug || opt_scrub_domheap;
         }
         else
         {