Re: [Xen-devel] [PATCH] x86/xpti: Hide almost all of .text and all .data/.rodata/.bss mappings

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.02.18 at 20:45, <andrew.cooper3@xxxxxxxxxx> wrote:
> RFC, because I don't think the stubs handling is particularly sensible.
> 
> We allocate 4k of virtual address space per CPU, but squash loads of CPUs
> together onto a single MFN.  The stubs ought to be isolated as well (as they
> leak the virtual addresses of each stack), which can be done by allocating an
> MFN per CPU (and simplifies cpu_smpboot_alloc() somewhat).  At this point, we
> can't use a common set of mappings, and will have to clone the single stub and
> .entry.text into each PCPUs copy of the pagetables.

The 4k-per-CPU allocation of VA space is probably not strictly
necessary - quoting the original commit message:
"While sharing physical pages among certain CPUs on the same node, for
 now the virtual mappings get established in distinct pages for each
 CPU. This isn't a strict requirement, but simplifies VA space
 management for this initial implementation: Sharing VA space would
 require additional tracking of which areas are currently in use. If
 the VA and/or TLB overhead turned out to be a problem, such extra code
 could easily be added."

Without allocating a page per CPU I think what you do is sensible.
And I don't see how hiding stubs of other CPUs would help much -
an attacker can gather stack locations from various CPUs as its
vCPU is being moved around, and you can't hide the current CPU's
stub space.

> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -622,6 +622,9 @@ unsigned long alloc_stub_page(unsigned int cpu, unsigned 
> long *mfn)
>          unmap_domain_page(memset(__map_domain_page(pg), 0xcc, PAGE_SIZE));
>      }
>  
> +    /* Confirm that all stubs fit in a single L2 pagetable. */
> +    BUILD_BUG_ON(NR_CPUS * PAGE_SIZE > (1u << L2_PAGETABLE_SHIFT));

Perhaps duplicate this in setup_cpu_root_pgt() (suitably adjusted
of course, as Jürgen has already pointed out)?

> @@ -651,9 +654,6 @@ static int clone_mapping(const void *ptr, root_pgentry_t 
> *rpt)
>      l2_pgentry_t *pl2e;
>      l1_pgentry_t *pl1e;
>  
> -    if ( linear < DIRECTMAP_VIRT_START )
> -        return 0;

Isn't outright removing this going a little too far?

> @@ -744,6 +744,9 @@ static __read_mostly int8_t opt_xpti = -1;
>  boolean_param("xpti", opt_xpti);
>  DEFINE_PER_CPU(root_pgentry_t *, root_pgt);
>  
> +static root_pgentry_t common_pgt;

Move into setup_cpu_root_pgt()?

> +extern char _stextentry[], _etextentry[];

const?

> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -13,6 +13,8 @@
>  #include <public/xen.h>
>  #include <irq_vectors.h>
>  
> +        .section .text.entry, "ax", @progbits
> +
>  ENTRY(entry_int82)
>          ASM_CLAC
>          pushq $0

This also puts compat_create_bounce_frame into the entry section,
which surely isn't needed. Same for the non-compat variant.

> @@ -854,7 +856,7 @@ GLOBAL(autogen_entrypoints)
>          .popsection
>          .endm
>  
> -        .text
> +        .previous
>  autogen_stubs: /* Automatically generated stubs. */

Perhaps better to switch the earlier .section to .pushsection, and
use .popsection here?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel