* [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 related [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 related [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 related [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).