| Message ID | 20240513134046.82605-7-eliasely@amazon.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Remove the directmap | expand |
On Mon, May 13, 2024 at 01:40:33PM +0000, Elias El Yandouzi wrote: > From: Hongyan Xia <hongyxia@amazon.com> > > Also add a helper function to retrieve it. Change arch_mfns_in_direct_map > to check this option before returning. > > This is added as a Kconfig option as well as a boot command line option. > While being generic, the Kconfig option is only usable for x86 at the moment. > > Note that there remains some users of the directmap at this point. The option > is introduced now as it will be needed in follow-up patches. It's hard for me to evaluate whether the option name and the help text is correct, because the implementation is not yet complete. It would be best if this was introduced after the implementation has gone in, so that the reviewer can evaluate that the text matches the implementation. Now it's mostly a promise of what's yet to be implemented. > Signed-off-by: Hongyan Xia <hongyxia@amazon.com> > Signed-off-by: Julien Grall <jgrall@amazon.com> > Signed-off-by: Elias El Yandouzi <eliasely@amazon.com> > > ---- > > Changes in V2: > * Introduce a Kconfig option > * Reword the commit message > * Make opt_directmap and helper generic > > Changes since Hongyan's version: > * Reword the commit message > * opt_directmap is only modified during boot so mark it as > __ro_after_init > > diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc > index e760f3266e..743d343ffa 100644 > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -799,6 +799,18 @@ that enabling this option cannot guarantee anything beyond what underlying > hardware guarantees (with, where available and known to Xen, respective > tweaks applied). > > +### directmap (x86) > +> `= <boolean>` > + > +> Default: `true` > + > +Enable or disable the directmap region in Xen. Enable or disable fully populating the directmap region in Xen. > + > +By default, Xen creates the directmap region which maps physical memory ^ all? > +in that region. Setting this to no will sparsely populate the directmap, "Setting this to no" => "Disabling this option will sparsely..." > +blocking exploits that leak secrets via speculative memory access in the > +directmap. > + > ### dma_bits > > `= <integer>` > > diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig > index 7e03e4bc55..b4ec0e582e 100644 > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -28,6 +28,7 @@ config X86 > select HAS_PCI_MSI > select HAS_PIRQ > select HAS_SCHED_GRANULARITY > + select HAS_SECRET_HIDING > select HAS_UBSAN > select HAS_VPCI if HVM > select NEEDS_LIBELF > diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h > index 98b66edaca..54d835f156 100644 > --- a/xen/arch/x86/include/asm/mm.h > +++ b/xen/arch/x86/include/asm/mm.h > @@ -622,11 +622,17 @@ void write_32bit_pse_identmap(uint32_t *l2); > /* > * x86 maps part of physical memory via the directmap region. > * Return whether the range of MFN falls in the directmap region. > + * > + * When boot command line sets directmap=no, the directmap will mostly be empty > + * so this will always return false. > */ > static inline bool arch_mfns_in_directmap(unsigned long mfn, unsigned long nr) > { > unsigned long eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END); > > + if ( !has_directmap() ) > + return false; > + > return (mfn + nr) <= (virt_to_mfn(eva - 1) + 1); > } > > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index f84e1cd79c..bd6b1184f5 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -1517,6 +1517,8 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) > if ( highmem_start ) > xenheap_max_mfn(PFN_DOWN(highmem_start - 1)); > > + printk("Booting with directmap %s\n", has_directmap() ? "on" : "off"); IMO this wants to be printed as part of the speculation mitigations, see print_details() in spec_ctrl.c > + > /* > * Walk every RAM region and map it in its entirety (on x86/64, at least) > * and notify it to the boot allocator. > diff --git a/xen/common/Kconfig b/xen/common/Kconfig > index 565ceda741..856604068c 100644 > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -80,12 +80,29 @@ config HAS_PMAP > config HAS_SCHED_GRANULARITY > bool > > +config HAS_SECRET_HIDING > + bool > + > config HAS_UBSAN > bool > > config MEM_ACCESS_ALWAYS_ON > bool > > +config SECRET_HIDING > + bool "Secret hiding" > + depends on HAS_SECRET_HIDING IMO 'SECRET_HIDING' is too generic, this wants a more specific name. Maybe SPARCE_DIRECTMAP or some such. > + help > + The directmap contains mapping for most of the RAM which makes domain > + memory easily accessible. While making the performance better, it also makes > + the hypervisor more vulnerable to speculation attacks. > + > + Enabling this feature will allow the user to decide whether the memory > + is always mapped at boot or mapped only on demand (see the command line > + option "directmap"). > + > + If unsure, say N. > + > config MEM_ACCESS > def_bool MEM_ACCESS_ALWAYS_ON > prompt "Memory Access and VM events" if !MEM_ACCESS_ALWAYS_ON > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index 7c1bdfc046..9b7e4721cd 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -174,6 +174,11 @@ paddr_t __ro_after_init mem_hotplug; > static char __initdata opt_badpage[100] = ""; > string_param("badpage", opt_badpage); > > +bool __ro_after_init opt_directmap = true; > +#ifdef CONFIG_HAS_SECRET_HIDING > +boolean_param("directmap", opt_directmap); > +#endif > + > /* > * no-bootscrub -> Free pages are not zeroed during boot. > */ > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h > index 7561297a75..9d4f1f2d0d 100644 > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -167,6 +167,13 @@ extern unsigned long max_page; > extern unsigned long total_pages; > extern paddr_t mem_hotplug; > > +extern bool opt_directmap; > + > +static inline bool has_directmap(void) > +{ > + return opt_directmap; This likely wants: return IS_ENABLED(CONFIG_HAS_SECRET_HIDING) && opt_directmap; So that when HAS_SECRET_HIDING is build time disabled the compiler can likely eliminate the code. Thanks, Roger.
On Tue, May 14, 2024 at 11:20:21AM +0200, Roger Pau Monné wrote: > On Mon, May 13, 2024 at 01:40:33PM +0000, Elias El Yandouzi wrote: > > From: Hongyan Xia <hongyxia@amazon.com> > > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h > > index 7561297a75..9d4f1f2d0d 100644 > > --- a/xen/include/xen/mm.h > > +++ b/xen/include/xen/mm.h > > @@ -167,6 +167,13 @@ extern unsigned long max_page; > > extern unsigned long total_pages; > > extern paddr_t mem_hotplug; > > > > +extern bool opt_directmap; > > + > > +static inline bool has_directmap(void) > > +{ > > + return opt_directmap; > > This likely wants: > > return IS_ENABLED(CONFIG_HAS_SECRET_HIDING) && opt_directmap; Er, sorry, this is wrong, should be: return !IS_ENABLED(CONFIG_HAS_SECRET_HIDING) || opt_directmap; Roger.
On 14.05.2024 11:20, Roger Pau Monné wrote: > On Mon, May 13, 2024 at 01:40:33PM +0000, Elias El Yandouzi wrote: >> --- a/docs/misc/xen-command-line.pandoc >> +++ b/docs/misc/xen-command-line.pandoc >> @@ -799,6 +799,18 @@ that enabling this option cannot guarantee anything beyond what underlying >> hardware guarantees (with, where available and known to Xen, respective >> tweaks applied). >> >> +### directmap (x86) >> +> `= <boolean>` >> + >> +> Default: `true` >> + >> +Enable or disable the directmap region in Xen. > > Enable or disable fully populating the directmap region in Xen. Elias, would you please take care to address earlier review comments before sending a new version? This and ... >> + >> +By default, Xen creates the directmap region which maps physical memory > ^ all? >> +in that region. Setting this to no will sparsely populate the directmap, > > "Setting this to no" => "Disabling this option will sparsely..." ... this is what I had already asked for in reply to v2, of course with different wording. >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -1517,6 +1517,8 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) >> if ( highmem_start ) >> xenheap_max_mfn(PFN_DOWN(highmem_start - 1)); >> >> + printk("Booting with directmap %s\n", has_directmap() ? "on" : "off"); > > IMO this wants to be printed as part of the speculation mitigations, see > print_details() in spec_ctrl.c And not "on" / "off", but "full" / "sparse" (and word order changed accordingly) perhaps. >> --- a/xen/common/Kconfig >> +++ b/xen/common/Kconfig >> @@ -80,12 +80,29 @@ config HAS_PMAP >> config HAS_SCHED_GRANULARITY >> bool >> >> +config HAS_SECRET_HIDING >> + bool >> + >> config HAS_UBSAN >> bool >> >> config MEM_ACCESS_ALWAYS_ON >> bool >> >> +config SECRET_HIDING >> + bool "Secret hiding" >> + depends on HAS_SECRET_HIDING > > IMO 'SECRET_HIDING' is too generic, this wants a more specific name. > Maybe SPARCE_DIRECTMAP or some such. This is another aspect I had raised on v2 already. SPARSE_DIRECTMAP would be fine with me. Jan
On 13.05.2024 15:40, Elias El Yandouzi wrote: > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -80,12 +80,29 @@ config HAS_PMAP > config HAS_SCHED_GRANULARITY > bool > > +config HAS_SECRET_HIDING > + bool > + > config HAS_UBSAN > bool > > config MEM_ACCESS_ALWAYS_ON > bool > > +config SECRET_HIDING > + bool "Secret hiding" > + depends on HAS_SECRET_HIDING > + help > + The directmap contains mapping for most of the RAM which makes domain > + memory easily accessible. While making the performance better, it also makes > + the hypervisor more vulnerable to speculation attacks. > + > + Enabling this feature will allow the user to decide whether the memory > + is always mapped at boot or mapped only on demand (see the command line > + option "directmap"). > + > + If unsure, say N. > + > config MEM_ACCESS > def_bool MEM_ACCESS_ALWAYS_ON > prompt "Memory Access and VM events" if !MEM_ACCESS_ALWAYS_ON Surely there's a better place to add this new setting than between two dependent options (MEM_ACCESS_ALWAYS_ON and MEM_ACCESS). > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -167,6 +167,13 @@ extern unsigned long max_page; > extern unsigned long total_pages; > extern paddr_t mem_hotplug; > > +extern bool opt_directmap; > + > +static inline bool has_directmap(void) > +{ > + return opt_directmap; > +} As indicated before, with the Kconfig setting off I think we want to have an alternative #define opt_directmap true There's no need to impact generated code by needing to look at a "variable" which is never going to change value. Jan
On 13.05.2024 15:40, Elias El Yandouzi wrote: > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -799,6 +799,18 @@ that enabling this option cannot guarantee anything beyond what underlying > hardware guarantees (with, where available and known to Xen, respective > tweaks applied). > > +### directmap (x86) > +> `= <boolean>` > + > +> Default: `true` > + > +Enable or disable the directmap region in Xen. > + > +By default, Xen creates the directmap region which maps physical memory > +in that region. Setting this to no will sparsely populate the directmap, > +blocking exploits that leak secrets via speculative memory access in the > +directmap. Along the lines of remarks on comments and descriptions: I think we ought to reserve "directmap=no" to a future where there really is the option of not having anything direct-mapped. Right now imo that option form ought to be invalid, and only "directmap=sparse" should be recognized (alongside "directmap=yes" of course). Jan
On Wed, May 15, 2024 at 03:54:51PM +0200, Jan Beulich wrote: > On 14.05.2024 11:20, Roger Pau Monné wrote: > > On Mon, May 13, 2024 at 01:40:33PM +0000, Elias El Yandouzi wrote: > >> --- a/docs/misc/xen-command-line.pandoc > >> +++ b/docs/misc/xen-command-line.pandoc > >> @@ -799,6 +799,18 @@ that enabling this option cannot guarantee anything beyond what underlying > >> hardware guarantees (with, where available and known to Xen, respective > >> tweaks applied). > >> > >> +### directmap (x86) > >> +> `= <boolean>` > >> + > >> +> Default: `true` > >> + > >> +Enable or disable the directmap region in Xen. > > > > Enable or disable fully populating the directmap region in Xen. > > Elias, would you please take care to address earlier review comments > before sending a new version? This and ... > > >> + > >> +By default, Xen creates the directmap region which maps physical memory > > ^ all? > >> +in that region. Setting this to no will sparsely populate the directmap, > > > > "Setting this to no" => "Disabling this option will sparsely..." > > ... this is what I had already asked for in reply to v2, of course with > different wording. > > >> --- a/xen/arch/x86/setup.c > >> +++ b/xen/arch/x86/setup.c > >> @@ -1517,6 +1517,8 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) > >> if ( highmem_start ) > >> xenheap_max_mfn(PFN_DOWN(highmem_start - 1)); > >> > >> + printk("Booting with directmap %s\n", has_directmap() ? "on" : "off"); > > > > IMO this wants to be printed as part of the speculation mitigations, see > > print_details() in spec_ctrl.c > > And not "on" / "off", but "full" / "sparse" (and word order changed accordingly) > perhaps. I've been thinking about this, and I'm leaning towards calling this new mode "ondemand" rather than "sparse". The fact that the direct map ends up sparely populated is a consequence of populating it on demand, and hence the later would be more descriptive IMO. (Same for the Kconfig option then ONDEMAND_DIRECTMAP, or some such) Thanks, Roger.
On 16.05.2024 11:19, Roger Pau Monné wrote: > On Wed, May 15, 2024 at 03:54:51PM +0200, Jan Beulich wrote: >> On 14.05.2024 11:20, Roger Pau Monné wrote: >>> On Mon, May 13, 2024 at 01:40:33PM +0000, Elias El Yandouzi wrote: >>>> --- a/docs/misc/xen-command-line.pandoc >>>> +++ b/docs/misc/xen-command-line.pandoc >>>> @@ -799,6 +799,18 @@ that enabling this option cannot guarantee anything beyond what underlying >>>> hardware guarantees (with, where available and known to Xen, respective >>>> tweaks applied). >>>> >>>> +### directmap (x86) >>>> +> `= <boolean>` >>>> + >>>> +> Default: `true` >>>> + >>>> +Enable or disable the directmap region in Xen. >>> >>> Enable or disable fully populating the directmap region in Xen. >> >> Elias, would you please take care to address earlier review comments >> before sending a new version? This and ... >> >>>> + >>>> +By default, Xen creates the directmap region which maps physical memory >>> ^ all? >>>> +in that region. Setting this to no will sparsely populate the directmap, >>> >>> "Setting this to no" => "Disabling this option will sparsely..." >> >> ... this is what I had already asked for in reply to v2, of course with >> different wording. >> >>>> --- a/xen/arch/x86/setup.c >>>> +++ b/xen/arch/x86/setup.c >>>> @@ -1517,6 +1517,8 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) >>>> if ( highmem_start ) >>>> xenheap_max_mfn(PFN_DOWN(highmem_start - 1)); >>>> >>>> + printk("Booting with directmap %s\n", has_directmap() ? "on" : "off"); >>> >>> IMO this wants to be printed as part of the speculation mitigations, see >>> print_details() in spec_ctrl.c >> >> And not "on" / "off", but "full" / "sparse" (and word order changed accordingly) >> perhaps. > > I've been thinking about this, and I'm leaning towards calling this > new mode "ondemand" rather than "sparse". The fact that the direct > map ends up sparely populated is a consequence of populating it on > demand, and hence the later would be more descriptive IMO. > > (Same for the Kconfig option then ONDEMAND_DIRECTMAP, or some such) Fine with me, fwiw. Jan
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index e760f3266e..743d343ffa 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -799,6 +799,18 @@ that enabling this option cannot guarantee anything beyond what underlying hardware guarantees (with, where available and known to Xen, respective tweaks applied). +### directmap (x86) +> `= <boolean>` + +> Default: `true` + +Enable or disable the directmap region in Xen. + +By default, Xen creates the directmap region which maps physical memory +in that region. Setting this to no will sparsely populate the directmap, +blocking exploits that leak secrets via speculative memory access in the +directmap. + ### dma_bits > `= <integer>` diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index 7e03e4bc55..b4ec0e582e 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -28,6 +28,7 @@ config X86 select HAS_PCI_MSI select HAS_PIRQ select HAS_SCHED_GRANULARITY + select HAS_SECRET_HIDING select HAS_UBSAN select HAS_VPCI if HVM select NEEDS_LIBELF diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h index 98b66edaca..54d835f156 100644 --- a/xen/arch/x86/include/asm/mm.h +++ b/xen/arch/x86/include/asm/mm.h @@ -622,11 +622,17 @@ void write_32bit_pse_identmap(uint32_t *l2); /* * x86 maps part of physical memory via the directmap region. * Return whether the range of MFN falls in the directmap region. + * + * When boot command line sets directmap=no, the directmap will mostly be empty + * so this will always return false. */ static inline bool arch_mfns_in_directmap(unsigned long mfn, unsigned long nr) { unsigned long eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END); + if ( !has_directmap() ) + return false; + return (mfn + nr) <= (virt_to_mfn(eva - 1) + 1); } diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index f84e1cd79c..bd6b1184f5 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1517,6 +1517,8 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) if ( highmem_start ) xenheap_max_mfn(PFN_DOWN(highmem_start - 1)); + printk("Booting with directmap %s\n", has_directmap() ? "on" : "off"); + /* * Walk every RAM region and map it in its entirety (on x86/64, at least) * and notify it to the boot allocator. diff --git a/xen/common/Kconfig b/xen/common/Kconfig index 565ceda741..856604068c 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -80,12 +80,29 @@ config HAS_PMAP config HAS_SCHED_GRANULARITY bool +config HAS_SECRET_HIDING + bool + config HAS_UBSAN bool config MEM_ACCESS_ALWAYS_ON bool +config SECRET_HIDING + bool "Secret hiding" + depends on HAS_SECRET_HIDING + help + The directmap contains mapping for most of the RAM which makes domain + memory easily accessible. While making the performance better, it also makes + the hypervisor more vulnerable to speculation attacks. + + Enabling this feature will allow the user to decide whether the memory + is always mapped at boot or mapped only on demand (see the command line + option "directmap"). + + If unsure, say N. + config MEM_ACCESS def_bool MEM_ACCESS_ALWAYS_ON prompt "Memory Access and VM events" if !MEM_ACCESS_ALWAYS_ON diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index 7c1bdfc046..9b7e4721cd 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -174,6 +174,11 @@ paddr_t __ro_after_init mem_hotplug; static char __initdata opt_badpage[100] = ""; string_param("badpage", opt_badpage); +bool __ro_after_init opt_directmap = true; +#ifdef CONFIG_HAS_SECRET_HIDING +boolean_param("directmap", opt_directmap); +#endif + /* * no-bootscrub -> Free pages are not zeroed during boot. */ diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index 7561297a75..9d4f1f2d0d 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -167,6 +167,13 @@ extern unsigned long max_page; extern unsigned long total_pages; extern paddr_t mem_hotplug; +extern bool opt_directmap; + +static inline bool has_directmap(void) +{ + return opt_directmap; +} + /* * Extra fault info types which are used to further describe * the source of an access violation.