xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] xen/x86: Remap text/data/bss with appropriate permissions
@ 2016-03-17 12:43 Andrew Cooper
  2016-03-17 12:43 ` [PATCH] DO NOT APPLY - debug keys for inspecting Xen mappings Andrew Cooper
  2016-03-17 14:31 ` [PATCH] xen/x86: Remap text/data/bss with appropriate permissions Jan Beulich
  0 siblings, 2 replies; 8+ messages in thread
From: Andrew Cooper @ 2016-03-17 12:43 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich

c/s cf39362 "x86: use 2M superpages for text/data/bss mappings" served two
purposes; to map the primary code and data with appropriate pagetable
permissions (rather than unilaterally RWX), and to reduce the TLB pressure.

The extra alignment exposed a SYSLinux issue, and was partly reverted by c/s
0b8a172 "x86: partially revert use of 2M mappings for hypervisor image".

This change reinstates the pagetable permission improvements while avoiding
the 2M alignment issue.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/setup.c   | 24 ++++++++++++++++++++++++
 xen/arch/x86/xen.lds.S | 16 ++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index c5c332d..e278a7b 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -529,9 +529,33 @@ static void noinline init_done(void)
     }
     else
     {
+        /* Mark .text as RX (avoiding the first 2M superpage). */
+        map_pages_to_xen(XEN_VIRT_START + MB(2),
+                         PFN_DOWN(__pa(XEN_VIRT_START + MB(2))),
+                         PFN_DOWN(__2M_text_end -
+                                  (const char *)(XEN_VIRT_START + MB(2))),
+                         PAGE_HYPERVISOR_RX);
+
+        /* Mark .rodata as RO. */
+        map_pages_to_xen((unsigned long)&__2M_rodata_start,
+                         PFN_DOWN(__pa(__2M_rodata_start)),
+                         PFN_DOWN(__2M_rodata_end - __2M_rodata_start),
+                         PAGE_HYPERVISOR_RO);
+
+        /* Free and reuse .init. */
         destroy_xen_mappings((unsigned long)&__init_begin,
                              (unsigned long)&__init_end);
         init_xenheap_pages(__pa(__init_begin), __pa(__init_end));
+
+        /* Mark .data and .bss as RW. */
+        map_pages_to_xen((unsigned long)&__2M_rwdata_start,
+                         PFN_DOWN(__pa(__2M_rwdata_start)),
+                         PFN_DOWN(__2M_rwdata_end - __2M_rwdata_start),
+                         PAGE_HYPERVISOR_RW);
+
+        /* Drop the remaining mappings in the shattered superpage. */
+        destroy_xen_mappings((unsigned long)&__2M_rwdata_end,
+                             ROUNDUP((unsigned long)&__2M_rwdata_end, MB(2)));
     }
 
     printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 961f48f..ab8ba74 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -55,6 +55,8 @@ SECTIONS
 
 #ifdef EFI
   . = ALIGN(MB(2));
+#else
+  . = ALIGN(PAGE_SIZE);
 #endif
   __2M_text_end = .;
 
@@ -98,6 +100,8 @@ SECTIONS
 
 #ifdef EFI
   . = ALIGN(MB(2));
+#else
+  . = ALIGN(PAGE_SIZE);
 #endif
   __2M_rodata_end = .;
 
@@ -167,6 +171,8 @@ SECTIONS
 
 #ifdef EFI
   . = ALIGN(MB(2));
+#else
+  . = ALIGN(PAGE_SIZE);
 #endif
   __2M_init_end = .;
 
@@ -211,6 +217,8 @@ SECTIONS
 
 #ifdef EFI
   . = ALIGN(MB(2));
+#else
+  . = ALIGN(PAGE_SIZE);
 #endif
   __2M_rwdata_end = .;
 
@@ -269,6 +277,14 @@ ASSERT(IS_ALIGNED(__2M_init_start,   MB(2)), "__2M_init_start misaligned")
 ASSERT(IS_ALIGNED(__2M_init_end,     MB(2)), "__2M_init_end misaligned")
 ASSERT(IS_ALIGNED(__2M_rwdata_start, MB(2)), "__2M_rwdata_start misaligned")
 ASSERT(IS_ALIGNED(__2M_rwdata_end,   MB(2)), "__2M_rwdata_end misaligned")
+#else
+ASSERT(IS_ALIGNED(__2M_text_end,     PAGE_SIZE), "__2M_text_end misaligned")
+ASSERT(IS_ALIGNED(__2M_rodata_start, PAGE_SIZE), "__2M_rodata_start misaligned")
+ASSERT(IS_ALIGNED(__2M_rodata_end,   PAGE_SIZE), "__2M_rodata_end misaligned")
+ASSERT(IS_ALIGNED(__2M_init_start,   PAGE_SIZE), "__2M_init_start misaligned")
+ASSERT(IS_ALIGNED(__2M_init_end,     PAGE_SIZE), "__2M_init_end misaligned")
+ASSERT(IS_ALIGNED(__2M_rwdata_start, PAGE_SIZE), "__2M_rwdata_start misaligned")
+ASSERT(IS_ALIGNED(__2M_rwdata_end,   PAGE_SIZE), "__2M_rwdata_end misaligned")
 #endif
 
 ASSERT(IS_ALIGNED(cpu0_stack, STACK_SIZE), "cpu0_stack misaligned")
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] DO NOT APPLY - debug keys for inspecting Xen mappings
  2016-03-17 12:43 [PATCH] xen/x86: Remap text/data/bss with appropriate permissions Andrew Cooper
@ 2016-03-17 12:43 ` Andrew Cooper
  2016-03-17 14:31 ` [PATCH] xen/x86: Remap text/data/bss with appropriate permissions Jan Beulich
  1 sibling, 0 replies; 8+ messages in thread
From: Andrew Cooper @ 2016-03-17 12:43 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper

---
 xen/arch/x86/mm.c    |  15 ++++++
 xen/arch/x86/traps.c | 148 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 163 insertions(+)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index c997b53..77752fe 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -123,6 +123,7 @@
 #include <asm/fixmap.h>
 #include <asm/io_apic.h>
 #include <asm/pci.h>
+#include <xen/console.h>
 
 /* Mapping of the fixmap space needed early. */
 l1_pgentry_t __section(".bss.page_aligned") l1_fixmap[L1_PAGETABLE_ENTRIES];
@@ -5635,6 +5636,20 @@ int map_pages_to_xen(
     l1_pgentry_t *pl1e, ol1e;
     unsigned int  i;
 
+    if ( XEN_VIRT_START >= virt &&
+         XEN_VIRT_START <= (virt + (nr_mfns * PAGE_SIZE)) )
+    {
+        console_start_sync();
+
+        printk("*** %s(%p, %p, %p, %#x)\n",
+               __func__, _p(virt), _p(mfn), _p(nr_mfns), flags);
+        printk("*** Called from %p %pS\n",
+               __builtin_return_address(0),
+               __builtin_return_address(0));
+
+        console_end_sync();
+    }
+
 #define flush_flags(oldf) do {                 \
     unsigned int o_ = (oldf);                  \
     if ( (o_) & _PAGE_GLOBAL )                 \
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 564a107..8381138 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -4124,6 +4124,154 @@ void asm_domain_crash_synchronous(unsigned long addr)
     __domain_crash_synchronous();
 }
 
+static const char *pte_flags_decode(unsigned int flags)
+{
+    static char buf[24];
+
+    snprintf(buf, sizeof buf, "%s%s%s%s%s%s",
+             flags & _PAGE_NX_BIT  ? "NX"   : "X",
+             flags & _PAGE_GLOBAL  ? " Gl"  : "",
+             flags & _PAGE_PSE     ? " +"   : "",
+             flags & _PAGE_USER    ? " U"   : " S",
+             flags & _PAGE_RW      ? " RW"  : " RO",
+             flags & _PAGE_PRESENT ? " P"   : ""
+        );
+    return buf;
+}
+
+void dump_xen_mappings(bool_t dump_l1)
+{
+    unsigned long cr3 = read_cr3();
+
+    unsigned int i4 = l4_table_offset(XEN_VIRT_START);
+    l4_pgentry_t *l4 = &idle_pg_table[i4];
+    unsigned int l4ef = l4e_get_flags(*l4);
+
+    unsigned int i3 = l3_table_offset(XEN_VIRT_START);
+    l3_pgentry_t *l3 = l4e_to_l3e(*l4);
+    unsigned int l3ef = l3e_get_flags(l3[i3]);
+
+    unsigned int i2;
+    l2_pgentry_t *l2 = l3e_to_l2e(l3[i3]);
+
+    printk("*** Dumping Xen text/data/bss mappings from %p\n",
+           _p(XEN_VIRT_START));
+
+    printk("cr3 %p, idle_pg_table %p, pa %p\n",
+           _p(cr3), idle_pg_table, _p(__pa(idle_pg_table)));
+
+    printk("l2_xenmap: %p, pa %p\n", l2_xenmap, _p(__pa(l2_xenmap)));
+
+    printk(" L4[%03u] = %"PRIpte" %s\n",
+           i4, l4e_get_intpte(*l4), pte_flags_decode(l4ef));
+
+    printk("  L3[%03u] = %"PRIpte" %s\n",
+           i3, l3e_get_intpte(l3[i3]), pte_flags_decode(l3ef));
+
+    if ( l3e_get_paddr(l3[i3]) != __pa(l2_xenmap) )
+        printk("** Unexpected - l3e not pointing at l2_xenmap\n");
+
+    for ( i2 = 0; i2 < L2_PAGETABLE_ENTRIES; ++i2 )
+    {
+        unsigned int l2ef = l2e_get_flags(l2[i2]), i1;
+        l1_pgentry_t *l1;
+
+        if ( !(l2ef & _PAGE_PRESENT) )
+            continue;
+
+        printk("   L2[%03u] = %"PRIpte" %s\n",
+               i2, l2e_get_intpte(l2[i2]), pte_flags_decode(l2ef));
+
+        if ( l2ef & _PAGE_PSE )
+            continue;
+
+        if ( !dump_l1 )
+        {
+            printk("    L1 abbr\n");
+            continue;
+        }
+
+        l1 = l2e_to_l1e(l2[i2]);
+
+        for ( i1 = 0; i1 < L1_PAGETABLE_ENTRIES; ++i1 )
+        {
+            unsigned int l1ef = l1e_get_flags(l1[i1]);
+
+            if ( !(l1ef & _PAGE_PRESENT) )
+                continue;
+
+            printk("    L1[%03u] = %"PRIpte" %s\n",
+                   i1, l1e_get_intpte(l1[i1]), pte_flags_decode(l1ef));
+        }
+    }
+}
+
+static void read_idle(void)
+{
+    l4_pgentry_t *l4 = idle_pg_table;
+    unsigned int i4;
+
+    for ( i4 = 0; i4 < L4_PAGETABLE_ENTRIES; ++i4 )
+    {
+        unsigned int l4ef = l4e_get_flags(l4[i4]);
+
+        if ( !(l4ef & _PAGE_PRESENT) )
+            continue;
+
+        printk(" L4[%03u] = %"PRIpte" %s\n",
+               i4, l4e_get_intpte(l4[i4]), pte_flags_decode(l4ef));
+    }
+}
+
+#include <xen/keyhandler.h>
+#include <asm/setup.h>
+
+static void dump_offsets(const char *name, unsigned long addr)
+{
+    printk("  %-15s: %lu %lu %lu %lu\n", name, l4_table_offset(addr),
+           l3_table_offset(addr), l2_table_offset(addr), l1_table_offset(addr));
+}
+#define DUMP(x) dump_offsets(#x, (unsigned long)&(x))
+
+static void do_extreme_debug(unsigned char key, struct cpu_user_regs *regs)
+{
+    printk("'%c' pressed -> Extreme debugging in progress...\n", key);
+
+    switch ( key )
+    {
+    case '1':
+        dump_xen_mappings(0);
+        break;
+
+    case '2':
+        dump_xen_mappings(1);
+        break;
+
+    case '3':
+        read_idle();
+        break;
+
+    case '4':
+        DUMP(_stext);
+        DUMP(_etext);
+        DUMP(__2M_rodata_start);
+        DUMP(__2M_rodata_end);
+        DUMP(__2M_rwdata_start);
+        DUMP(__2M_rwdata_end);
+        break;
+    }
+}
+
+static int __init extreme_debug_keyhandler_init(void)
+{
+    register_irq_keyhandler('1', &do_extreme_debug, "Extreme debugging 1", 0);
+    register_irq_keyhandler('2', &do_extreme_debug, "Extreme debugging 2", 0);
+    register_irq_keyhandler('3', &do_extreme_debug, "Extreme debugging 3", 0);
+    register_irq_keyhandler('4', &do_extreme_debug, "Extreme debugging 3", 0);
+    return 0;
+}
+__initcall(extreme_debug_keyhandler_init);
+
 /*
  * Local variables:
  * mode: C
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] xen/x86: Remap text/data/bss with appropriate permissions
  2016-03-17 12:43 [PATCH] xen/x86: Remap text/data/bss with appropriate permissions Andrew Cooper
  2016-03-17 12:43 ` [PATCH] DO NOT APPLY - debug keys for inspecting Xen mappings Andrew Cooper
@ 2016-03-17 14:31 ` Jan Beulich
  2016-03-17 14:44   ` Andrew Cooper
  2016-03-18 19:49   ` [PATCH v2] " Andrew Cooper
  1 sibling, 2 replies; 8+ messages in thread
From: Jan Beulich @ 2016-03-17 14:31 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel

>>> On 17.03.16 at 13:43, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -529,9 +529,33 @@ static void noinline init_done(void)
>      }
>      else
>      {
> +        /* Mark .text as RX (avoiding the first 2M superpage). */
> +        map_pages_to_xen(XEN_VIRT_START + MB(2),
> +                         PFN_DOWN(__pa(XEN_VIRT_START + MB(2))),
> +                         PFN_DOWN(__2M_text_end -
> +                                  (const char *)(XEN_VIRT_START + MB(2))),
> +                         PAGE_HYPERVISOR_RX);
> +
> +        /* Mark .rodata as RO. */
> +        map_pages_to_xen((unsigned long)&__2M_rodata_start,
> +                         PFN_DOWN(__pa(__2M_rodata_start)),
> +                         PFN_DOWN(__2M_rodata_end - __2M_rodata_start),
> +                         PAGE_HYPERVISOR_RO);
> +
> +        /* Free and reuse .init. */
>          destroy_xen_mappings((unsigned long)&__init_begin,
>                               (unsigned long)&__init_end);
>          init_xenheap_pages(__pa(__init_begin), __pa(__init_end));
> +
> +        /* Mark .data and .bss as RW. */
> +        map_pages_to_xen((unsigned long)&__2M_rwdata_start,
> +                         PFN_DOWN(__pa(__2M_rwdata_start)),
> +                         PFN_DOWN(__2M_rwdata_end - __2M_rwdata_start),
> +                         PAGE_HYPERVISOR_RW);
> +
> +        /* Drop the remaining mappings in the shattered superpage. */
> +        destroy_xen_mappings((unsigned long)&__2M_rwdata_end,
> +                             ROUNDUP((unsigned long)&__2M_rwdata_end, MB(2)));
>      }

I think this would be more appropriate to add to some __init
function (which the discarding of .init.* naturally can't live in).

Also - do we really want to make this code dependent on
map_pages_to_xen() not intermediately zapping the mappings
being changed?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] xen/x86: Remap text/data/bss with appropriate permissions
  2016-03-17 14:31 ` [PATCH] xen/x86: Remap text/data/bss with appropriate permissions Jan Beulich
@ 2016-03-17 14:44   ` Andrew Cooper
  2016-03-17 15:32     ` Jan Beulich
  2016-03-18 19:49   ` [PATCH v2] " Andrew Cooper
  1 sibling, 1 reply; 8+ messages in thread
From: Andrew Cooper @ 2016-03-17 14:44 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Xen-devel

On 17/03/16 14:31, Jan Beulich wrote:
>>>> On 17.03.16 at 13:43, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -529,9 +529,33 @@ static void noinline init_done(void)
>>      }
>>      else
>>      {
>> +        /* Mark .text as RX (avoiding the first 2M superpage). */
>> +        map_pages_to_xen(XEN_VIRT_START + MB(2),
>> +                         PFN_DOWN(__pa(XEN_VIRT_START + MB(2))),
>> +                         PFN_DOWN(__2M_text_end -
>> +                                  (const char *)(XEN_VIRT_START + MB(2))),
>> +                         PAGE_HYPERVISOR_RX);
>> +
>> +        /* Mark .rodata as RO. */
>> +        map_pages_to_xen((unsigned long)&__2M_rodata_start,
>> +                         PFN_DOWN(__pa(__2M_rodata_start)),
>> +                         PFN_DOWN(__2M_rodata_end - __2M_rodata_start),
>> +                         PAGE_HYPERVISOR_RO);
>> +
>> +        /* Free and reuse .init. */
>>          destroy_xen_mappings((unsigned long)&__init_begin,
>>                               (unsigned long)&__init_end);
>>          init_xenheap_pages(__pa(__init_begin), __pa(__init_end));
>> +
>> +        /* Mark .data and .bss as RW. */
>> +        map_pages_to_xen((unsigned long)&__2M_rwdata_start,
>> +                         PFN_DOWN(__pa(__2M_rwdata_start)),
>> +                         PFN_DOWN(__2M_rwdata_end - __2M_rwdata_start),
>> +                         PAGE_HYPERVISOR_RW);
>> +
>> +        /* Drop the remaining mappings in the shattered superpage. */
>> +        destroy_xen_mappings((unsigned long)&__2M_rwdata_end,
>> +                             ROUNDUP((unsigned long)&__2M_rwdata_end, MB(2)));
>>      }
> I think this would be more appropriate to add to some __init
> function (which the discarding of .init.* naturally can't live in).

I will see if I can pull it forwards to just after the relocation, to be
as close to the permissions change in the 2M case as possible.

>
> Also - do we really want to make this code dependent on
> map_pages_to_xen() not intermediately zapping the mappings
> being changed?

Do you mean "immediately"?

As far as I can tell, it is guaranteed to be safe, even when remapping
the code section.  Updates to the live pagetables are using atomic
writes, and I didn't spot a point which would end up with a transient
non-present mapping.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] xen/x86: Remap text/data/bss with appropriate permissions
  2016-03-17 14:44   ` Andrew Cooper
@ 2016-03-17 15:32     ` Jan Beulich
  2016-03-17 16:15       ` Andrew Cooper
  0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2016-03-17 15:32 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel

>>> On 17.03.16 at 15:44, <andrew.cooper3@citrix.com> wrote:
> On 17/03/16 14:31, Jan Beulich wrote:
>> Also - do we really want to make this code dependent on
>> map_pages_to_xen() not intermediately zapping the mappings
>> being changed?
> 
> Do you mean "immediately"?

No.

> As far as I can tell, it is guaranteed to be safe, even when remapping
> the code section.  Updates to the live pagetables are using atomic
> writes, and I didn't spot a point which would end up with a transient
> non-present mapping.

But we may, at some point and for whatever reason, come to make
the function zap the mapping (i.e. clear the present bit), flush, and
only the re-establish the new mapping.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] xen/x86: Remap text/data/bss with appropriate permissions
  2016-03-17 15:32     ` Jan Beulich
@ 2016-03-17 16:15       ` Andrew Cooper
  2016-03-18  7:25         ` Jan Beulich
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Cooper @ 2016-03-17 16:15 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Xen-devel

On 17/03/16 15:32, Jan Beulich wrote:
>>>> On 17.03.16 at 15:44, <andrew.cooper3@citrix.com> wrote:
>> On 17/03/16 14:31, Jan Beulich wrote:
>>> Also - do we really want to make this code dependent on
>>> map_pages_to_xen() not intermediately zapping the mappings
>>> being changed?
>> Do you mean "immediately"?
> No.
>
>> As far as I can tell, it is guaranteed to be safe, even when remapping
>> the code section.  Updates to the live pagetables are using atomic
>> writes, and I didn't spot a point which would end up with a transient
>> non-present mapping.
> But we may, at some point and for whatever reason, come to make
> the function zap the mapping (i.e. clear the present bit), flush, and
> only the re-establish the new mapping.

This change is temporary until I can fix the legacy boot issue and
reintroduce the proper 2M functionality.

If someone in the future wants to change the behaviour of
map_pages_to_xen() then we can reconsider.  However, I think it is
unlikely that this will actually happen at all, and if it ever does, I
hope to have already fixed the 2M alignment and deleted this change.

This change is a big security improvement, and absolutely should be
taken, especially as the current implementation of map_pages_to_xen() is
safe.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] xen/x86: Remap text/data/bss with appropriate permissions
  2016-03-17 16:15       ` Andrew Cooper
@ 2016-03-18  7:25         ` Jan Beulich
  0 siblings, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2016-03-18  7:25 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel

>>> On 17.03.16 at 17:15, <andrew.cooper3@citrix.com> wrote:
> On 17/03/16 15:32, Jan Beulich wrote:
>>>>> On 17.03.16 at 15:44, <andrew.cooper3@citrix.com> wrote:
>>> On 17/03/16 14:31, Jan Beulich wrote:
>>>> Also - do we really want to make this code dependent on
>>>> map_pages_to_xen() not intermediately zapping the mappings
>>>> being changed?
>>> Do you mean "immediately"?
>> No.
>>
>>> As far as I can tell, it is guaranteed to be safe, even when remapping
>>> the code section.  Updates to the live pagetables are using atomic
>>> writes, and I didn't spot a point which would end up with a transient
>>> non-present mapping.
>> But we may, at some point and for whatever reason, come to make
>> the function zap the mapping (i.e. clear the present bit), flush, and
>> only the re-establish the new mapping.
> 
> This change is temporary until I can fix the legacy boot issue and
> reintroduce the proper 2M functionality.
> 
> If someone in the future wants to change the behaviour of
> map_pages_to_xen() then we can reconsider.  However, I think it is
> unlikely that this will actually happen at all, and if it ever does, I
> hope to have already fixed the 2M alignment and deleted this change.
> 
> This change is a big security improvement, and absolutely should be
> taken, especially as the current implementation of map_pages_to_xen() is
> safe.

I by no means intend to reject this change just because of this
aspect - I merely wanted to make the slight concern explicit.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] xen/x86: Remap text/data/bss with appropriate permissions
  2016-03-17 14:31 ` [PATCH] xen/x86: Remap text/data/bss with appropriate permissions Jan Beulich
  2016-03-17 14:44   ` Andrew Cooper
@ 2016-03-18 19:49   ` Andrew Cooper
  1 sibling, 0 replies; 8+ messages in thread
From: Andrew Cooper @ 2016-03-18 19:49 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich

c/s cf39362 "x86: use 2M superpages for text/data/bss mappings" served two
purposes; to map the primary code and data with appropriate pagetable
permissions (rather than unilaterally RWX), and to reduce the TLB pressure.

The extra alignment exposed a SYSLinux issue, and was partly reverted by c/s
0b8a172 "x86: partially revert use of 2M mappings for hypervisor image".

This change reinstates the pagetable permission improvements while avoiding
the 2M alignment issue.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>

v2:

Move the remapping earlier in the boot sequence.  This is where
memguart_init() used to reside, which itself used map_pages_to_xen() on the
full virtual region.
---
 xen/arch/x86/setup.c   | 31 +++++++++++++++++++++++++++++++
 xen/arch/x86/xen.lds.S | 16 ++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 1876a28..ee65f55 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1205,6 +1205,37 @@ void __init noreturn __start_xen(unsigned long mbi_p)
                    ~((1UL << L2_PAGETABLE_SHIFT) - 1);
     destroy_xen_mappings(xen_virt_end, XEN_VIRT_START + BOOTSTRAP_MAP_BASE);
 
+    /*
+     * If not using 2M mappings to gain suitable pagetable permissions
+     * directly from the relocation above, remap the code/data
+     * sections with decreased permissions.
+     */
+    if ( !using_2M_mapping() )
+    {
+        /* Mark .text as RX (avoiding the first 2M superpage). */
+        map_pages_to_xen(XEN_VIRT_START + MB(2),
+                         PFN_DOWN(__pa(XEN_VIRT_START + MB(2))),
+                         PFN_DOWN(__2M_text_end -
+                                  (const char *)(XEN_VIRT_START + MB(2))),
+                         PAGE_HYPERVISOR_RX);
+
+        /* Mark .rodata as RO. */
+        map_pages_to_xen((unsigned long)&__2M_rodata_start,
+                         PFN_DOWN(__pa(__2M_rodata_start)),
+                         PFN_DOWN(__2M_rodata_end - __2M_rodata_start),
+                         PAGE_HYPERVISOR_RO);
+
+        /* Mark .data and .bss as RW. */
+        map_pages_to_xen((unsigned long)&__2M_rwdata_start,
+                         PFN_DOWN(__pa(__2M_rwdata_start)),
+                         PFN_DOWN(__2M_rwdata_end - __2M_rwdata_start),
+                         PAGE_HYPERVISOR_RW);
+
+        /* Drop the remaining mappings in the shattered superpage. */
+        destroy_xen_mappings((unsigned long)&__2M_rwdata_end,
+                             ROUNDUP((unsigned long)&__2M_rwdata_end, MB(2)));
+    }
+
     nr_pages = 0;
     for ( i = 0; i < e820.nr_map; i++ )
         if ( e820.map[i].type == E820_RAM )
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index e47771c..5eb825e 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -56,6 +56,8 @@ SECTIONS
 
 #ifdef EFI
   . = ALIGN(MB(2));
+#else
+  . = ALIGN(PAGE_SIZE);
 #endif
   __2M_text_end = .;
 
@@ -99,6 +101,8 @@ SECTIONS
 
 #ifdef EFI
   . = ALIGN(MB(2));
+#else
+  . = ALIGN(PAGE_SIZE);
 #endif
   __2M_rodata_end = .;
 
@@ -168,6 +172,8 @@ SECTIONS
 
 #ifdef EFI
   . = ALIGN(MB(2));
+#else
+  . = ALIGN(PAGE_SIZE);
 #endif
   __2M_init_end = .;
 
@@ -212,6 +218,8 @@ SECTIONS
 
 #ifdef EFI
   . = ALIGN(MB(2));
+#else
+  . = ALIGN(PAGE_SIZE);
 #endif
   __2M_rwdata_end = .;
 
@@ -270,6 +278,14 @@ ASSERT(IS_ALIGNED(__2M_init_start,   MB(2)), "__2M_init_start misaligned")
 ASSERT(IS_ALIGNED(__2M_init_end,     MB(2)), "__2M_init_end misaligned")
 ASSERT(IS_ALIGNED(__2M_rwdata_start, MB(2)), "__2M_rwdata_start misaligned")
 ASSERT(IS_ALIGNED(__2M_rwdata_end,   MB(2)), "__2M_rwdata_end misaligned")
+#else
+ASSERT(IS_ALIGNED(__2M_text_end,     PAGE_SIZE), "__2M_text_end misaligned")
+ASSERT(IS_ALIGNED(__2M_rodata_start, PAGE_SIZE), "__2M_rodata_start misaligned")
+ASSERT(IS_ALIGNED(__2M_rodata_end,   PAGE_SIZE), "__2M_rodata_end misaligned")
+ASSERT(IS_ALIGNED(__2M_init_start,   PAGE_SIZE), "__2M_init_start misaligned")
+ASSERT(IS_ALIGNED(__2M_init_end,     PAGE_SIZE), "__2M_init_end misaligned")
+ASSERT(IS_ALIGNED(__2M_rwdata_start, PAGE_SIZE), "__2M_rwdata_start misaligned")
+ASSERT(IS_ALIGNED(__2M_rwdata_end,   PAGE_SIZE), "__2M_rwdata_end misaligned")
 #endif
 
 ASSERT(IS_ALIGNED(cpu0_stack, STACK_SIZE), "cpu0_stack misaligned")
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-03-18 19:49 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-17 12:43 [PATCH] xen/x86: Remap text/data/bss with appropriate permissions Andrew Cooper
2016-03-17 12:43 ` [PATCH] DO NOT APPLY - debug keys for inspecting Xen mappings Andrew Cooper
2016-03-17 14:31 ` [PATCH] xen/x86: Remap text/data/bss with appropriate permissions Jan Beulich
2016-03-17 14:44   ` Andrew Cooper
2016-03-17 15:32     ` Jan Beulich
2016-03-17 16:15       ` Andrew Cooper
2016-03-18  7:25         ` Jan Beulich
2016-03-18 19:49   ` [PATCH v2] " Andrew Cooper

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).