| Message ID | 20190506124624.54454-1-elnikety@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mm: option to _always_ scrub freed domheap pages | expand |
On 5/6/19 1:46 PM, Eslam Elnikety wrote: > 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 <elnikety@amazon.com> Now that I think about it -- Andy, isn't there a patch in the XenServer patchqueue to enable scrubbing by default? I'm wondering if this should default to 'true', and people who really want the extra performance should turn it off. Only one other minor comment: > --- > docs/misc/xen-command-line.pandoc | 8 ++++++++ > xen/common/page_alloc.c | 11 +++++++++-- > 2 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc > index 7dcb22932a..5a92949c5a 100644 > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -270,6 +270,14 @@ and not running softirqs. Reduce this if softirqs are not being run frequently > enough. Setting this to a high value may cause boot failure, particularly if > the NMI watchdog is also enabled. > > +### 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. > + > ### clocksource (x86) > > `= pit | hpet | acpi | tsc` > > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index be44158033..678a00ac9b 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -214,6 +214,12 @@ 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_t opt_scrub_domheap = 0; > +boolean_param("scrub_domheap", opt_scrub_domheap); I'm sure Jan will request this to be 'scrub-domheap' instead (not using '_' when you can use '-'). Otherwise this looks good to me: Acked-by: George Dunlap <george.dunlap@citrix.com> I think both of these could probably be fixed up on check-in. -George
On Tue, May 07, 2019 at 10:55:51AM +0100, George Dunlap wrote: > On 5/6/19 1:46 PM, Eslam Elnikety wrote: > > 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 <elnikety@amazon.com> > > Now that I think about it -- Andy, isn't there a patch in the XenServer > patchqueue to enable scrubbing by default? > > I'm wondering if this should default to 'true', and people who really > want the extra performance should turn it off. > > Only one other minor comment: > > > --- > > docs/misc/xen-command-line.pandoc | 8 ++++++++ > > xen/common/page_alloc.c | 11 +++++++++-- > > 2 files changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc > > index 7dcb22932a..5a92949c5a 100644 > > --- a/docs/misc/xen-command-line.pandoc > > +++ b/docs/misc/xen-command-line.pandoc > > @@ -270,6 +270,14 @@ and not running softirqs. Reduce this if softirqs are not being run frequently > > enough. Setting this to a high value may cause boot failure, particularly if > > the NMI watchdog is also enabled. > > > > +### 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. > > + > > ### clocksource (x86) > > > `= pit | hpet | acpi | tsc` > > > > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > > index be44158033..678a00ac9b 100644 > > --- a/xen/common/page_alloc.c > > +++ b/xen/common/page_alloc.c > > @@ -214,6 +214,12 @@ 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_t opt_scrub_domheap = 0; Please change bool_t to bool and 0 to false while you're at it. :-) Wei.
>>> On 07.05.19 at 11:55, <george.dunlap@citrix.com> wrote: > On 5/6/19 1:46 PM, Eslam Elnikety wrote: >[...] > I'm wondering if this should default to 'true', and people who really > want the extra performance should turn it off. Why would we want to cater for buggy guests by default? >> --- a/docs/misc/xen-command-line.pandoc >> +++ b/docs/misc/xen-command-line.pandoc >> @@ -270,6 +270,14 @@ and not running softirqs. Reduce this if softirqs are not being run frequently >> enough. Setting this to a high value may cause boot failure, particularly if >> the NMI watchdog is also enabled. >> >> +### 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. >> + >> ### clocksource (x86) >> > `= pit | hpet | acpi | tsc` The entries here are alphabetically sorted, so the addition wants to move down quite a bit. >> --- a/xen/common/page_alloc.c >> +++ b/xen/common/page_alloc.c >> @@ -214,6 +214,12 @@ 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_t opt_scrub_domheap = 0; >> +boolean_param("scrub_domheap", opt_scrub_domheap); > > I'm sure Jan will request this to be 'scrub-domheap' instead (not using > '_' when you can use '-'). Indeed, plus use "bool", drop the pointless initializer, and correct the style of the (single line) comment. Jan
>>> On 07.05.19 at 12:03, <JBeulich@suse.com> wrote: >>>> On 07.05.19 at 11:55, <george.dunlap@citrix.com> wrote: >> On 5/6/19 1:46 PM, Eslam Elnikety wrote: >>> --- a/xen/common/page_alloc.c >>> +++ b/xen/common/page_alloc.c >>> @@ -214,6 +214,12 @@ 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_t opt_scrub_domheap = 0; >>> +boolean_param("scrub_domheap", opt_scrub_domheap); >> >> I'm sure Jan will request this to be 'scrub-domheap' instead (not using >> '_' when you can use '-'). > > Indeed, plus use "bool", drop the pointless initializer, and correct > the style of the (single line) comment. And use __read_mostly. Jan
> On 7. May 2019, at 12:06, Jan Beulich <jbeulich@suse.com> wrote: > >>>> On 07.05.19 at 12:03, <JBeulich@suse.com> wrote: >>>>> On 07.05.19 at 11:55, <george.dunlap@citrix.com> wrote: >>> On 5/6/19 1:46 PM, Eslam Elnikety wrote: >>>> --- a/xen/common/page_alloc.c >>>> +++ b/xen/common/page_alloc.c >>>> @@ -214,6 +214,12 @@ 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_t opt_scrub_domheap = 0; >>>> +boolean_param("scrub_domheap", opt_scrub_domheap); >>> >>> I'm sure Jan will request this to be 'scrub-domheap' instead (not using >>> '_' when you can use '-'). >> >> Indeed, plus use "bool", drop the pointless initializer, and correct >> the style of the (single line) comment. > > And use __read_mostly. > > Jan > > Thanks for all the comments on this thread. v2 should take care of all those comments. Cheers, Eslam
On 5/7/19 11:03 AM, Jan Beulich wrote: >>>> On 07.05.19 at 11:55, <george.dunlap@citrix.com> wrote: >> On 5/6/19 1:46 PM, Eslam Elnikety wrote: >> [...] >> I'm wondering if this should default to 'true', and people who really >> want the extra performance should turn it off. > > Why would we want to cater for buggy guests by default? Because one of the big selling points for Xen is that it is more secure; and one of the points of virtualization is to run guests for which you have no option of changing or upgrading. Having an extra safety catch to prevent loss of data *even in the case of buggy guests* on by default is on-brand for us. That said, the suggestion was predicated on my rather vague impression that XenServer has been doing this by default for years now, and thus has been proven to not be terribly problematic performance-wise. If that's not the case, then off-by-default might be a better option. -George
On 07/05/2019 10:55, George Dunlap wrote: > On 5/6/19 1:46 PM, Eslam Elnikety wrote: >> 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 <elnikety@amazon.com> > Now that I think about it -- Andy, isn't there a patch in the XenServer > patchqueue to enable scrubbing by default? > > I'm wondering if this should default to 'true', and people who really > want the extra performance should turn it off. https://github.com/xenserver/xen.pg/blob/XS-8.0.x/master/0001-cc-memory-scrubbing.patch We couldn't measure a performance difference with unconditional scrubbing, so chose to do without an extra parameter to tweak. ~Andrew
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index 7dcb22932a..5a92949c5a 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -270,6 +270,14 @@ and not running softirqs. Reduce this if softirqs are not being run frequently enough. Setting this to a high value may cause boot failure, particularly if the NMI watchdog is also enabled. +### 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. + ### clocksource (x86) > `= pit | hpet | acpi | tsc` diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index be44158033..678a00ac9b 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -214,6 +214,12 @@ 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_t opt_scrub_domheap = 0; +boolean_param("scrub_domheap", opt_scrub_domheap); + #ifdef CONFIG_SCRUB_DEBUG static bool __read_mostly scrub_debug; #else @@ -2378,9 +2384,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 {
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 <elnikety@amazon.com> --- docs/misc/xen-command-line.pandoc | 8 ++++++++ xen/common/page_alloc.c | 11 +++++++++-- 2 files changed, 17 insertions(+), 2 deletions(-)