Re: [PATCH] x86/xpti: really hide almost all of Xen image

Re: [PATCH] x86/xpti: really hide almost all of Xen image

[Juergen Gross]

On 02/03/18 12:23, Jan Beulich wrote:
> Commit 422588e885 ("x86/xpti: Hide almost all of .text and all
> .data/.rodata/.bss mappings") carefully limited the Xen image cloning to
> just entry code, but then overwrote the just allocated and populated L3
> entry with the normal one again covering both Xen image and stubs.
> 
> Drop the respective code in favor of an explicit clone_mapping()
> invocation. This in turn now requires setup_cpu_root_pgt() to run after
> stub setup in all cases. Additionally, with (almost) no unintended
> mappings left, the BSP's IDT now also needs to be page aligned.
> 
> Note that the removed BUILD_BUG_ON()s don't get replaced by anything -
> there already is a suitable ASSERT() in xen.lds.S.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Tested-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Juergen Gross <jgross@suse.com>

> ---
> What should we do with the TSS? Currently together with it we expose
> almost a full page of other per-CPU data. A simple (but slightly
> hackish) option would be to use one of the two unused stack slots.

Either one of the unused stack pages or directly after the GDT (we could
then drop NR_RESERVED_GDT_PAGES and reduce NR_RESERVED_GDT_ENTRIES to
a lower value, e.g. 16 or 32).

> Talking of the stack: While APs properly have the guard page mirrored
> into the cloned page tables, this is a debug-build only thing _and_
> doesn't cover the BSP. Should we perhaps add code to fully mirror the
> guard pages, including on release builds?

+1


Juergen

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

Re: [PATCH] x86/xpti: really hide almost all of Xen image

[Jan Beulich]

>>> On 02.03.18 at 13:16, <jgross@suse.com> wrote:
> On 02/03/18 12:23, Jan Beulich wrote:
>> Commit 422588e885 ("x86/xpti: Hide almost all of .text and all
>> .data/.rodata/.bss mappings") carefully limited the Xen image cloning to
>> just entry code, but then overwrote the just allocated and populated L3
>> entry with the normal one again covering both Xen image and stubs.
>> 
>> Drop the respective code in favor of an explicit clone_mapping()
>> invocation. This in turn now requires setup_cpu_root_pgt() to run after
>> stub setup in all cases. Additionally, with (almost) no unintended
>> mappings left, the BSP's IDT now also needs to be page aligned.
>> 
>> Note that the removed BUILD_BUG_ON()s don't get replaced by anything -
>> there already is a suitable ASSERT() in xen.lds.S.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Tested-by: Juergen Gross <jgross@suse.com>
> Reviewed-by: Juergen Gross <jgross@suse.com>

Thanks, but I'm afraid there's a minor bug in that change: I need
to free the page tables that the new clone_mapping() may have
produced, but I need to do that without affecting common_pgt.
It may therefore be worthwhile considering to retain the
original approach instead, just doing the changes at L2 rather
than L3. Andrew, do you have a preference either way?

>> ---
>> What should we do with the TSS? Currently together with it we expose
>> almost a full page of other per-CPU data. A simple (but slightly
>> hackish) option would be to use one of the two unused stack slots.
> 
> Either one of the unused stack pages or directly after the GDT (we could
> then drop NR_RESERVED_GDT_PAGES and reduce NR_RESERVED_GDT_ENTRIES to
> a lower value, e.g. 16 or 32).

I dislike moving the TSS into per-domain space. There shouldn't be
anything there that isn't per-domain.

Jan


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

[PATCH] x86/xpti: really hide almost all of Xen image

[Jan Beulich]

Commit 422588e885 ("x86/xpti: Hide almost all of .text and all
.data/.rodata/.bss mappings") carefully limited the Xen image cloning to
just entry code, but then overwrote the just allocated and populated L3
entry with the normal one again covering both Xen image and stubs.

Drop the respective code in favor of an explicit clone_mapping()
invocation. This in turn now requires setup_cpu_root_pgt() to run after
stub setup in all cases. Additionally, with (almost) no unintended
mappings left, the BSP's IDT now also needs to be page aligned.

Note that the removed BUILD_BUG_ON()s don't get replaced by anything -
there already is a suitable ASSERT() in xen.lds.S.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
What should we do with the TSS? Currently together with it we expose
almost a full page of other per-CPU data. A simple (but slightly
hackish) option would be to use one of the two unused stack slots.

Talking of the stack: While APs properly have the guard page mirrored
into the cloned page tables, this is a debug-build only thing _and_
doesn't cover the BSP. Should we perhaps add code to fully mirror the
guard pages, including on release builds?

--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -622,9 +622,6 @@ unsigned long alloc_stub_page(unsigned i
         unmap_domain_page(memset(__map_domain_page(pg), 0xcc, PAGE_SIZE));
     }
 
-    /* Confirm that all stubs fit in a single L3 entry. */
-    BUILD_BUG_ON(NR_CPUS * PAGE_SIZE > (1u << L3_PAGETABLE_SHIFT));
-
     stub_va = XEN_VIRT_END - (cpu + 1) * PAGE_SIZE;
     if ( map_pages_to_xen(stub_va, mfn_x(page_to_mfn(pg)), 1,
                           PAGE_HYPERVISOR_RX | MAP_SMALL_PAGES) )
@@ -786,8 +783,6 @@ static int setup_cpu_root_pgt(unsigned i
     /* One-time setup of common_pgt, which maps .text.entry and the stubs. */
     if ( unlikely(!root_get_intpte(common_pgt)) )
     {
-        unsigned long stubs_linear = XEN_VIRT_END - 1;
-        l3_pgentry_t *stubs_main, *stubs_shadow;
         const char *ptr;
 
         for ( rc = 0, ptr = _stextentry;
@@ -797,16 +792,6 @@ static int setup_cpu_root_pgt(unsigned i
         if ( rc )
             return rc;
 
-        /* Confirm that all stubs fit in a single L3 entry. */
-        BUILD_BUG_ON(NR_CPUS * PAGE_SIZE > (1u << L3_PAGETABLE_SHIFT));
-
-        stubs_main = l4e_to_l3e(idle_pg_table[l4_table_offset(stubs_linear)]);
-        stubs_shadow = l4e_to_l3e(rpt[l4_table_offset(stubs_linear)]);
-
-        /* Splice into the regular L2 mapping the stubs. */
-        stubs_shadow[l3_table_offset(stubs_linear)] =
-            stubs_main[l3_table_offset(stubs_linear)];
-
         common_pgt = rpt[root_table_offset(XEN_VIRT_START)];
     }
 
@@ -820,6 +805,8 @@ static int setup_cpu_root_pgt(unsigned i
         rc = clone_mapping(idt_tables[cpu], rpt);
     if ( !rc )
         rc = clone_mapping(&per_cpu(init_tss, cpu), rpt);
+    if ( !rc )
+        rc = clone_mapping((void *)per_cpu(stubs.addr, cpu), rpt);
 
     return rc;
 }
@@ -968,11 +955,6 @@ static int cpu_smpboot_alloc(unsigned in
     memcpy(idt_tables[cpu], idt_table, IDT_ENTRIES * sizeof(idt_entry_t));
     disable_each_ist(idt_tables[cpu]);
 
-    rc = setup_cpu_root_pgt(cpu);
-    if ( rc )
-        goto out;
-    rc = -ENOMEM;
-
     for ( stub_page = 0, i = cpu & ~(STUBS_PER_PAGE - 1);
           i < nr_cpu_ids && i <= (cpu | (STUBS_PER_PAGE - 1)); ++i )
         if ( cpu_online(i) && cpu_to_node(i) == node )
@@ -986,6 +968,11 @@ static int cpu_smpboot_alloc(unsigned in
         goto out;
     per_cpu(stubs.addr, cpu) = stub_page + STUB_BUF_CPU_OFFS(cpu);
 
+    rc = setup_cpu_root_pgt(cpu);
+    if ( rc )
+        goto out;
+    rc = -ENOMEM;
+
     if ( secondary_socket_cpumask == NULL &&
          (secondary_socket_cpumask = xzalloc(cpumask_t)) == NULL )
         goto out;
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -102,7 +102,8 @@ DEFINE_PER_CPU_READ_MOSTLY(struct desc_s
 DEFINE_PER_CPU_READ_MOSTLY(struct desc_struct *, compat_gdt_table);
 
 /* Master table, used by CPU0. */
-idt_entry_t idt_table[IDT_ENTRIES];
+idt_entry_t __section(".data.page_aligned") __aligned(PAGE_SIZE)
+    idt_table[IDT_ENTRIES];
 
 /* Pointer to the IDT of every CPU. */
 idt_entry_t *idt_tables[NR_CPUS] __read_mostly;



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