| Message ID | 20240116192611.41112-14-eliasely@amazon.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Remove the directmap | expand |
On 16.01.2024 20:25, 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 direct map region in Xen. > + > +By default, Xen creates the direct map region which maps physical memory > +in that region. Setting this to no will remove the direct map, blocking > +exploits that leak secrets via speculative memory access in the direct > +map. I think this wants wording such that the full truth is conveyed: The directmap doesn't disappear. It's merely only sparsely populated then. > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -29,6 +29,7 @@ config X86 > select HAS_UBSAN > select HAS_VPCI if HVM > select NEEDS_LIBELF > + select HAS_SECRET_HIDING Please respect alphabetic sorting. As to "secret hiding" - personally I consider this too generic a term. This is about limiting the direct map. Why not name the option then accordingly? > --- a/xen/arch/x86/include/asm/mm.h > +++ b/xen/arch/x86/include/asm/mm.h > @@ -620,10 +620,18 @@ 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, we will not have a direct map at > + * all so this will always return false. > */ As with the command line doc, please state the full truth. > static inline bool arch_mfns_in_directmap(unsigned long mfn, unsigned long nr) > { > - unsigned long eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END); > + unsigned long eva; > + > + if ( !has_directmap() ) > + return false; Hmm. The sole user of this function is init_node_heap(). Would it perhaps make sense to simply map the indicated number of pages then? init_node_heap() would fall back to xmalloc(), so the data will be in what's left of the directmap anyway. > + eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END); Irrespective I don't see a need to replace the initializer by an assignment. > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -83,6 +83,23 @@ config HAS_UBSAN > config MEM_ACCESS_ALWAYS_ON > bool > > +config HAS_SECRET_HIDING > + bool This again wants placing suitably among the other HAS_*. > +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. Nit: Indentation and no ---help--- anymore (just help) please in new Kconfig entries. Also as an alternative did you consider making this new setting merely control the default of opt_directmap? Otherwise the variable shouldn't exist at all when the Kconfig option is off, but rather be #define-d to "true" in that case. > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -165,6 +165,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; > +} If opt_directmap isn't static, I see little point in having such a wrapper. If there are reasons, I think they want stating in the description. On the whole: Is the placement of this patch in the series an indication that as of here all directmap uses have gone away? If so, what's the rest of the series about? Alternatively isn't use of this option still problematic at this point of the series? Whichever way it is - this wants clarifying in the description. Jan
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index 8e65f8bd18..63c946f482 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 direct map region in Xen. + +By default, Xen creates the direct map region which maps physical memory +in that region. Setting this to no will remove the direct map, blocking +exploits that leak secrets via speculative memory access in the direct +map. + ### dma_bits > `= <integer>` diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index 1acdffc51c..350f41b832 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -29,6 +29,7 @@ config X86 select HAS_UBSAN select HAS_VPCI if HVM select NEEDS_LIBELF + select HAS_SECRET_HIDING config ARCH_DEFCONFIG string diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h index 7d26d9cd2f..4aae270a78 100644 --- a/xen/arch/x86/include/asm/mm.h +++ b/xen/arch/x86/include/asm/mm.h @@ -620,10 +620,18 @@ 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, we will not have a direct map at + * all 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); + unsigned long eva; + + if ( !has_directmap() ) + return false; + + eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END); return (mfn + nr) <= (virt_to_mfn(eva - 1) + 1); } diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 4d0c90b7a0..b813ea75b5 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1512,6 +1512,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 310ad4229c..9a24c89ac5 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -83,6 +83,23 @@ config HAS_UBSAN config MEM_ACCESS_ALWAYS_ON bool +config HAS_SECRET_HIDING + 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 740b6f0ff7..a3746cfbcf 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -173,6 +173,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 3d9b2d05a5..f860e98ee4 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -165,6 +165,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.