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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.02.18 at 17:23, <andrew.cooper3@xxxxxxxxxx> wrote:
> The current XPTI implementation isolates the directmap (and therefore a lot 
> of
> guest data), but a large quantity of CPU0's state (including its stack)
> remains visible.
> 
> Furthermore, an attacker able to read .text is in a vastly superior position
> to normal when it comes to fingerprinting Xen for known vulnerabilities, or
> scanning for ROP/Spectre gadgets.
> 
> Collect together the entrypoints in .text.entry (currently 3x4k frames, but
> can almost certainly be slimmed down), and create a common mapping which is
> inserted into each per-cpu shadow.  The stubs are also inserted into this
> mapping by pointing at the in-use L2.  This allows stubs allocated later (SMP
> boot, or CPU hotplug) to work without further changes to the common 
> mappings.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Juergen Gross <jgross@xxxxxxxx>
> 
> v2:
>  * Drop "incomplete" warning from the docs.  This is about as complete as it
>    can reasonably get.
>  * Correct BUILD_BUG_ON() calculation, duplicate in the common_pgt code
>  * scope/const improvements
>  * Use .push/.popsection in preference to .previous
>  * Exclude {compat_}create_bounce_frame from .text.entry
>  * Extend the sanity checking of linear in clone_mapping().  There is a 
> latent
>    bug where a bad linear parameter would cause Xen to fall over a NULL 
> pointer.
> ---
>  docs/misc/xen-command-line.markdown |  3 --
>  xen/arch/x86/smpboot.c              | 56 
> +++++++++++++++++++++++++++++++++----
>  xen/arch/x86/x86_64/compat/entry.S  |  5 ++++
>  xen/arch/x86/x86_64/entry.S         | 11 ++++++--
>  xen/arch/x86/xen.lds.S              |  7 +++++
>  5 files changed, 71 insertions(+), 11 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.markdown 
> b/docs/misc/xen-command-line.markdown
> index 79feba6..8317639 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1935,9 +1935,6 @@ mode.
>  Override default selection of whether to isolate 64-bit PV guest page
>  tables.
>  
> -** WARNING: Not yet a complete isolation implementation, but better than
> -nothing. **
> -
>  ### xsave
>  > `= <boolean>`
>  
> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
> index 2ebef03..10bf2f3 100644
> --- 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 << L3_PAGETABLE_SHIFT));

It's a little confusing that the comment talks about L2 yet the check
is on L3. May I suggest you either use
L2_PAGETABLE_ENTRIES << L2_PAGETABLE_SHIFT or say "are
covered by a single L3 entry" in the comment?

> @@ -646,13 +649,23 @@ static int clone_mapping(const void *ptr, 
> root_pgentry_t *rpt)
>  {
>      unsigned long linear = (unsigned long)ptr, pfn;
>      unsigned int flags;
> -    l3_pgentry_t *pl3e = 
> l4e_to_l3e(idle_pg_table[root_table_offset(linear)]) +
> -                         l3_table_offset(linear);
> +    l3_pgentry_t *pl3e;
>      l2_pgentry_t *pl2e;
>      l1_pgentry_t *pl1e;
>  
> -    if ( linear < DIRECTMAP_VIRT_START )
> -        return 0;
> +    /*
> +     * Sanity check 'linear'.  We only allow cloning from the Xen virtual
> +     * range, and in particular, only from the directmap and .text ranges.
> +     */
> +    if ( root_table_offset(linear) > ROOT_PAGETABLE_LAST_XEN_SLOT ||
> +         root_table_offset(linear) < ROOT_PAGETABLE_FIRST_XEN_SLOT )
> +        return -EINVAL;
> +    if ( !(linear >= DIRECTMAP_VIRT_START ||
> +           (linear >= XEN_VIRT_START && linear < XEN_VIRT_END)) )

Putting it this way is certainly fine, but generally I find using !() on
logical || or && expressions less easy to understand than pushing the
negation through the entire expression. Once having done so, it
might even be debatable whether

    if ( linear < XEN_VIRT_START ||
         (linear >= XEN_VIRT_END && linear < DIRECTMAP_VIRT_START)) )
        return -EINVAL;

wouldn't be better.

With at least the first (comment vs code) aspect taken care of in
both instances
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan

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