x86: don't default to executable mappings

PATCH In review Jan Beulich 2 participants 14 messages May 21, 2015 – May 22, 2015

Share & tools

Jan Beulich

author

2015-05-21 10:07 UTC #0 lore raw

v2 cover x86: don't default to executable mappings

Particularly for the 1:1 mapping it was pointed out that in order to limit the damage from security issues we should avoid mapping things executable when they don't need to be.

1: move syscall trampolines off the stack
2: emul: move stubs off the stack
3: move I/O emulation stubs off the stack
4: switch default mapping attributes to non-executable

Signed-off-by Jan Beulich <[email protected]>

Jan Beulich

author

2015-05-21 10:15 UTC #1 lore raw

PATCH v2 1/4 x86: move syscall trampolines off the stack

This is needed as stacks are going to become non-executable. Use separate stub pages (shared among suitable CPUs on the same node) instead.

Stub areas (currently 128 bytes each) are being split into two parts - a fixed usage one (the syscall ones) and dynamically usable space, which will be used by subsequent changes to hold dynamically generated code during instruction eumlation.

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.

Signed-off-by Jan Beulich <[email protected]>

---
v2: Get rid of bogus VA adjustment in alloc_stub_page(). Introduce
STUB_BUF_CPU_OFFS(). Use ~PAGE_MASK instead of PAGE_SIZE - 1 for page offset masking. Comment adjustments. Extend description.

xen/arch/x86/setup.c +4 -0

@@ -1270,6 +1270,10 @@ void __init noreturn __start_xen(unsigne
 
     init_idle_domain();
 
+    this_cpu(stubs.addr) = alloc_stub_page(smp_processor_id(),
+                                           &this_cpu(stubs).mfn);
+    BUG_ON(!this_cpu(stubs.addr));
+
     trap_init();
 
     rcu_init();

xen/arch/x86/smpboot.c +68 -1

101 lines not on this page · raw message

xen/arch/x86/x86_64/compat/entry.S +14 -1

@@ -219,7 +219,20 @@ ENTRY(compat_post_handle_exception)
         movb  $0,TRAPBOUNCE_flags(%rdx)
         jmp   compat_test_all_events
 
-ENTRY(compat_syscall)
+/* See lstar_enter for entry register state. */
+ENTRY(cstar_enter)
+        sti
+        movq  8(%rsp),%rax /* Restore %rax. */
+        movq  $FLAT_KERNEL_SS,8(%rsp)
+        pushq %r11
+        pushq $FLAT_USER_CS32
+        pushq %rcx
+        pushq $0
+        SAVE_VOLATILE TRAP_syscall
+        GET_CURRENT(%rbx)
+        movq  VCPU_domain(%rbx),%rcx
+        cmpb  $0,DOMAIN_is_32bit_pv(%rcx)
+        je    switch_to_kernel
         cmpb  $0,VCPU_syscall32_disables_events(%rbx)
         movzwl VCPU_syscall32_sel(%rbx),%esi
         movq  VCPU_syscall32_addr(%rbx),%rax

xen/arch/x86/x86_64/entry.S +11 ~~-13~~

@@ -13,9 +13,8 @@
 #include <public/xen.h>
 #include <irq_vectors.h>
 
-        ALIGN
 /* %rbx: struct vcpu */
-switch_to_kernel:
+ENTRY(switch_to_kernel)
         leaq  VCPU_trap_bounce(%rbx),%rdx
         /* TB_eip = (32-bit syscall && syscall32_addr) ?
          *          syscall32_addr : syscall_addr */

@@ -113,23 +112,22 @@ restore_all_xen:
  * When entering SYSCALL from user mode:
  *  Vector directly to the registered arch.syscall_addr.
  *
- * Initial work is done by per-CPU stack trampolines. At this point %rsp
- * has been initialised to point at the correct Xen stack, and %rsp, %rflags
- * and %cs have been saved. All other registers are still to be saved onto
- * the stack, starting with %rip, and an appropriate %ss must be saved into
- * the space left by the trampoline.
+ * Initial work is done by per-CPU trampolines. At this point %rsp has been
+ * initialised to point at the correct Xen stack, %rsp has been saved, and
+ * %rax needs to be restored from the %ss save slot. All other registers are
+ * still to be saved onto the stack, starting with RFLAGS, and an appropriate
+ * %ss must be saved into the space left by the trampoline.
  */
-ENTRY(syscall_enter)
+ENTRY(lstar_enter)
         sti
-        movl  $FLAT_KERNEL_SS,24(%rsp)
+        movq  8(%rsp),%rax /* Restore %rax. */
+        movq  $FLAT_KERNEL_SS,8(%rsp)
+        pushq %r11
+        pushq $FLAT_KERNEL_CS64
         pushq %rcx
         pushq $0
-        movq  24(%rsp),%r11 /* Re-load user RFLAGS into %r11 before saving */
         SAVE_VOLATILE TRAP_syscall
         GET_CURRENT(%rbx)
-        movq  VCPU_domain(%rbx),%rcx
-        testb $1,DOMAIN_is_32bit_pv(%rcx)
-        jnz   compat_syscall
         testb $TF_kernel_mode,VCPU_thread_flags(%rbx)
         jz    switch_to_kernel

xen/arch/x86/x86_64/traps.c +52 ~~-44~~

122 lines not on this page · raw message

xen/arch/x86/xen.lds.S +3 -0

@@ -217,4 +217,7 @@ SECTIONS
   .comment 0 : { *(.comment) }
 }
 
+ASSERT(__image_base__ > XEN_VIRT_START ||
+       _end <= XEN_VIRT_END - NR_CPUS * PAGE_SIZE,
+       "Xen image overlaps stubs area")
 ASSERT(kexec_reloc_size - kexec_reloc <= PAGE_SIZE, "kexec_reloc is too large")

xen/include/asm-x86/config.h +4 -0

@@ -94,6 +94,10 @@
 /* Primary stack is restricted to 8kB by guard pages. */
 #define PRIMARY_STACK_SIZE 8192
 
+/* Total size of syscall and emulation stubs. */
+#define STUB_BUF_SHIFT max(L1_CACHE_SHIFT, 7)
+#define STUB_BUF_SIZE  (1 << STUB_BUF_SHIFT)
+
 /* Return value for zero-size _xmalloc(), distinguished from NULL. */
 #define ZERO_BLOCK_PTR ((void *)0xBAD0BAD0BAD0BAD0UL)

xen/include/asm-x86/page.h +4 -4

@@ -321,10 +321,10 @@ void efi_update_l4_pgtable(unsigned int 
 #define _PAGE_GNTTAB   0
 #endif
 
-#define __PAGE_HYPERVISOR \
-    (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED)
-#define __PAGE_HYPERVISOR_NOCACHE \
-    (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_PCD | _PAGE_ACCESSED)
+#define __PAGE_HYPERVISOR_RX      (_PAGE_PRESENT | _PAGE_ACCESSED)
+#define __PAGE_HYPERVISOR         (__PAGE_HYPERVISOR_RX | \
+                                   _PAGE_DIRTY | _PAGE_RW)
+#define __PAGE_HYPERVISOR_NOCACHE (__PAGE_HYPERVISOR | _PAGE_PCD)
 
 #define MAP_SMALL_PAGES _PAGE_AVAIL0 /* don't use superpages mappings */

xen/include/asm-x86/processor.h +13 -1

@@ -532,12 +532,24 @@ void trap_nop(void);
 void enable_nmis(void);
 void do_reserved_trap(struct cpu_user_regs *regs);
 
-void syscall_enter(void);
 void sysenter_entry(void);
 void sysenter_eflags_saved(void);
 void compat_hypercall(void);
 void int80_direct_trap(void);
 
+#define STUBS_PER_PAGE (PAGE_SIZE / STUB_BUF_SIZE)
+
+struct stubs {
+    union {
+        void(*func)(void);
+        unsigned long addr;
+    };
+    unsigned long mfn;
+};
+
+DECLARE_PER_CPU(struct stubs, stubs);
+unsigned long alloc_stub_page(unsigned int cpu, unsigned long *mfn);
+
 extern int hypercall(void);
 
 int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,

xen/include/asm-x86/x86_64/page.h +1 -0

@@ -148,6 +148,7 @@ typedef l4_pgentry_t root_pgentry_t;
 #define _PAGE_GUEST_KERNEL (1U<<12)
 
 #define PAGE_HYPERVISOR         (__PAGE_HYPERVISOR         | _PAGE_GLOBAL)
+#define PAGE_HYPERVISOR_RX      (__PAGE_HYPERVISOR_RX      | _PAGE_GLOBAL)
 #define PAGE_HYPERVISOR_NOCACHE (__PAGE_HYPERVISOR_NOCACHE | _PAGE_GLOBAL)
 
 #endif /* __X86_64_PAGE_H__ */

x86: move syscall trampolines off the stack

This is needed as stacks are going to become non-executable. Use separate stub pages (shared among suitable CPUs on the same node) instead.

Signed-off-by Jan Beulich <[email protected]>

xen/arch/x86/setup.c +4 -0

@@ -1270,6 +1270,10 @@ void __init noreturn __start_xen(unsigne
 
     init_idle_domain();
 
+    this_cpu(stubs.addr) = alloc_stub_page(smp_processor_id(),
+                                           &this_cpu(stubs).mfn);
+    BUG_ON(!this_cpu(stubs.addr));
+
     trap_init();
 
     rcu_init();

xen/arch/x86/smpboot.c +68 -1

101 lines not on this page · raw message

xen/arch/x86/x86_64/compat/entry.S +14 -1

@@ -219,7 +219,20 @@ ENTRY(compat_post_handle_exception)
         movb  $0,TRAPBOUNCE_flags(%rdx)
         jmp   compat_test_all_events
 
-ENTRY(compat_syscall)
+/* See lstar_enter for entry register state. */
+ENTRY(cstar_enter)
+        sti
+        movq  8(%rsp),%rax /* Restore %rax. */
+        movq  $FLAT_KERNEL_SS,8(%rsp)
+        pushq %r11
+        pushq $FLAT_USER_CS32
+        pushq %rcx
+        pushq $0
+        SAVE_VOLATILE TRAP_syscall
+        GET_CURRENT(%rbx)
+        movq  VCPU_domain(%rbx),%rcx
+        cmpb  $0,DOMAIN_is_32bit_pv(%rcx)
+        je    switch_to_kernel
         cmpb  $0,VCPU_syscall32_disables_events(%rbx)
         movzwl VCPU_syscall32_sel(%rbx),%esi
         movq  VCPU_syscall32_addr(%rbx),%rax

xen/arch/x86/x86_64/entry.S +11 ~~-13~~

@@ -13,9 +13,8 @@
 #include <public/xen.h>
 #include <irq_vectors.h>
 
-        ALIGN
 /* %rbx: struct vcpu */
-switch_to_kernel:
+ENTRY(switch_to_kernel)
         leaq  VCPU_trap_bounce(%rbx),%rdx
         /* TB_eip = (32-bit syscall && syscall32_addr) ?
          *          syscall32_addr : syscall_addr */

@@ -113,23 +112,22 @@ restore_all_xen:
  * When entering SYSCALL from user mode:
  *  Vector directly to the registered arch.syscall_addr.
  *
- * Initial work is done by per-CPU stack trampolines. At this point %rsp
- * has been initialised to point at the correct Xen stack, and %rsp, %rflags
- * and %cs have been saved. All other registers are still to be saved onto
- * the stack, starting with %rip, and an appropriate %ss must be saved into
- * the space left by the trampoline.
+ * Initial work is done by per-CPU trampolines. At this point %rsp has been
+ * initialised to point at the correct Xen stack, %rsp has been saved, and
+ * %rax needs to be restored from the %ss save slot. All other registers are
+ * still to be saved onto the stack, starting with RFLAGS, and an appropriate
+ * %ss must be saved into the space left by the trampoline.
  */
-ENTRY(syscall_enter)
+ENTRY(lstar_enter)
         sti
-        movl  $FLAT_KERNEL_SS,24(%rsp)
+        movq  8(%rsp),%rax /* Restore %rax. */
+        movq  $FLAT_KERNEL_SS,8(%rsp)
+        pushq %r11
+        pushq $FLAT_KERNEL_CS64
         pushq %rcx
         pushq $0
-        movq  24(%rsp),%r11 /* Re-load user RFLAGS into %r11 before saving */
         SAVE_VOLATILE TRAP_syscall
         GET_CURRENT(%rbx)
-        movq  VCPU_domain(%rbx),%rcx
-        testb $1,DOMAIN_is_32bit_pv(%rcx)
-        jnz   compat_syscall
         testb $TF_kernel_mode,VCPU_thread_flags(%rbx)
         jz    switch_to_kernel

xen/arch/x86/x86_64/traps.c +52 ~~-44~~

122 lines not on this page · raw message

xen/arch/x86/xen.lds.S +3 -0

@@ -217,4 +217,7 @@ SECTIONS
   .comment 0 : { *(.comment) }
 }
 
+ASSERT(__image_base__ > XEN_VIRT_START ||
+       _end <= XEN_VIRT_END - NR_CPUS * PAGE_SIZE,
+       "Xen image overlaps stubs area")
 ASSERT(kexec_reloc_size - kexec_reloc <= PAGE_SIZE, "kexec_reloc is too large")

xen/include/asm-x86/config.h +4 -0

@@ -94,6 +94,10 @@
 /* Primary stack is restricted to 8kB by guard pages. */
 #define PRIMARY_STACK_SIZE 8192
 
+/* Total size of syscall and emulation stubs. */
+#define STUB_BUF_SHIFT max(L1_CACHE_SHIFT, 7)
+#define STUB_BUF_SIZE  (1 << STUB_BUF_SHIFT)
+
 /* Return value for zero-size _xmalloc(), distinguished from NULL. */
 #define ZERO_BLOCK_PTR ((void *)0xBAD0BAD0BAD0BAD0UL)

xen/include/asm-x86/page.h +4 -4

@@ -321,10 +321,10 @@ void efi_update_l4_pgtable(unsigned int 
 #define _PAGE_GNTTAB   0
 #endif
 
-#define __PAGE_HYPERVISOR \
-    (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED)
-#define __PAGE_HYPERVISOR_NOCACHE \
-    (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_PCD | _PAGE_ACCESSED)
+#define __PAGE_HYPERVISOR_RX      (_PAGE_PRESENT | _PAGE_ACCESSED)
+#define __PAGE_HYPERVISOR         (__PAGE_HYPERVISOR_RX | \
+                                   _PAGE_DIRTY | _PAGE_RW)
+#define __PAGE_HYPERVISOR_NOCACHE (__PAGE_HYPERVISOR | _PAGE_PCD)
 
 #define MAP_SMALL_PAGES _PAGE_AVAIL0 /* don't use superpages mappings */

xen/include/asm-x86/processor.h +13 -1

@@ -532,12 +532,24 @@ void trap_nop(void);
 void enable_nmis(void);
 void do_reserved_trap(struct cpu_user_regs *regs);
 
-void syscall_enter(void);
 void sysenter_entry(void);
 void sysenter_eflags_saved(void);
 void compat_hypercall(void);
 void int80_direct_trap(void);
 
+#define STUBS_PER_PAGE (PAGE_SIZE / STUB_BUF_SIZE)
+
+struct stubs {
+    union {
+        void(*func)(void);
+        unsigned long addr;
+    };
+    unsigned long mfn;
+};
+
+DECLARE_PER_CPU(struct stubs, stubs);
+unsigned long alloc_stub_page(unsigned int cpu, unsigned long *mfn);
+
 extern int hypercall(void);
 
 int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,

xen/include/asm-x86/x86_64/page.h +1 -0

@@ -148,6 +148,7 @@ typedef l4_pgentry_t root_pgentry_t;
 #define _PAGE_GUEST_KERNEL (1U<<12)
 
 #define PAGE_HYPERVISOR         (__PAGE_HYPERVISOR         | _PAGE_GLOBAL)
+#define PAGE_HYPERVISOR_RX      (__PAGE_HYPERVISOR_RX      | _PAGE_GLOBAL)
 #define PAGE_HYPERVISOR_NOCACHE (__PAGE_HYPERVISOR_NOCACHE | _PAGE_GLOBAL)
 
 #endif /* __X86_64_PAGE_H__ */

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Andrew Cooper

Reviewed-by on v2 1/4

2015-05-21 11:08 UTC #2 lore raw

Re: [PATCH v2 1/4] x86: move syscall trampolines off the stack

Quoted 17 lines · Jan Beulich

This is needed as stacks are going to become non-executable. Use separate stub pages (shared among suitable CPUs on the same node) instead.
Stub areas (currently 128 bytes each) are being split into two parts - a fixed usage one (the syscall ones) and dynamically usable space, which will be used by subsequent changes to hold dynamically generated code during instruction eumlation.
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.

Quoted 1 line

Signed-off-by Jan Beulich <[email protected]>

Reviewed-by Andrew Cooper <[email protected]>

It might be wise to have a BUILD_BUG_ON() which confirms that STUBS_PER_PAGE is a power of two, which is a requirement given the way it is used.

Jan Beulich

author

2015-05-21 11:48 UTC #3 lore raw

Re: [PATCH v2 1/4] x86: move syscall trampolines off the stack

Quoted 19 lines

Quoted 1 line
Quoted 1 line · On 21.05.15 at 13:08, <[email protected]> wrote:
Quoted 17 lines · Jan Beulich
This is needed as stacks are going to become non-executable. Use separate stub pages (shared among suitable CPUs on the same node) instead.
Stub areas (currently 128 bytes each) are being split into two parts - a fixed usage one (the syscall ones) and dynamically usable space, which will be used by subsequent changes to hold dynamically generated code during instruction eumlation.
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.

Quoted 7 lines

Quoted 1 line
Signed-off-by Jan Beulich <[email protected]>
Reviewed-by Andrew Cooper <[email protected]>
It might be wise to have a BUILD_BUG_ON() which confirms that STUBS_PER_PAGE is a power of two, which is a requirement given the way it is used.

Good idea. Along with that one I'm also adding

    BUILD_BUG_ON(STUB_BUF_SIZE / 2 < MAX_INST_LEN + 1);

to patch 2 and

    BUILD_BUG_ON(STUB_BUF_SIZE / 2 < 16);

to patch 3, in the hope that this won't invalidate your R-b.

Jan

Andrew Cooper

2015-05-21 11:56 UTC #4 lore raw

Re: [PATCH v2 1/4] x86: move syscall trampolines off the stack

Quoted 27 lines · Jan Beulich

Quoted 19 lines
Quoted 1 line
Quoted 1 line · On 21.05.15 at 13:08, <[email protected]> wrote:
Quoted 17 lines · Jan Beulich
This is needed as stacks are going to become non-executable. Use separate stub pages (shared among suitable CPUs on the same node) instead.
Stub areas (currently 128 bytes each) are being split into two parts - a fixed usage one (the syscall ones) and dynamically usable space, which will be used by subsequent changes to hold dynamically generated code during instruction eumlation.
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.
Quoted 6 lines
Quoted 1 line
Signed-off-by Jan Beulich <[email protected]>
Reviewed-by Andrew Cooper <[email protected]>
It might be wise to have a BUILD_BUG_ON() which confirms that STUBS_PER_PAGE is a power of two, which is a requirement given the way it is used.
Good idea. Along with that one I'm also adding

Quoted 7 lines

    BUILD_BUG_ON(STUB_BUF_SIZE / 2 < MAX_INST_LEN + 1);
to patch 2 and
    BUILD_BUG_ON(STUB_BUF_SIZE / 2 < 16);
to patch 3, in the hope that this won't invalidate your R-b.

Both fine by me

~Andrew

Jan Beulich

author

2015-05-22 08:25 UTC #5 lore raw

Re: [PATCH v2 1/4] x86: move syscall trampolines off the stack

In reply to Jan Beulich

Quoted 22 lines

Quoted 22 lines
Quoted 2 lines · On 21.05.15 at 13:48, <[email protected]> wrote:
Quoted 1 line · On 21.05.15 at 13:08, <[email protected]> wrote:
Quoted 18 lines · Jan Beulich
This is needed as stacks are going to become non-executable. Use separate stub pages (shared among suitable CPUs on the same node) instead.
Stub areas (currently 128 bytes each) are being split into two parts - a fixed usage one (the syscall ones) and dynamically usable space, which will be used by subsequent changes to hold dynamically generated code during instruction eumlation.
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.
Signed-off-by Jan Beulich <[email protected]>

Quoted 7 lines

Quoted 5 lines
Reviewed-by Andrew Cooper <[email protected]>
It might be wise to have a BUILD_BUG_ON() which confirms that STUBS_PER_PAGE is a power of two, which is a requirement given the way it is used.
Good idea.

Sadly this can't be a BUILD_BUG_ON(), as STUBS_PER_PAGE isn't a compile time constant (due to the use of max()). I made it an ASSERT() for the time being.

Jan

Jan Beulich

author

2015-05-22 08:34 UTC #6 lore raw

Re: [PATCH v2 1/4] x86: move syscall trampolines off the stack

Quoted 7 lines

Quoted 7 lines
Quoted 6 lines · On 22.05.15 at 10:25, <[email protected]> wrote:
Quoted 2 lines · On 21.05.15 at 13:48, <[email protected]> wrote:
Quoted 1 line · On 21.05.15 at 13:08, <[email protected]> wrote:
It might be wise to have a BUILD_BUG_ON() which confirms that STUBS_PER_PAGE is a power of two, which is a requirement given the way it is used.

Quoted 5 lines

Quoted 1 line
Good idea.
Sadly this can't be a BUILD_BUG_ON(), as STUBS_PER_PAGE isn't a compile time constant (due to the use of max()). I made it an ASSERT() for the time being.

Or actually no, since the other two would need to become ASSERT()s too, I'll rather open code the max() instead.

Jan

Andrew Cooper

2015-05-22 08:35 UTC #7 lore raw

Re: [PATCH v2 1/4] x86: move syscall trampolines off the stack

In reply to Jan Beulich

Quoted 23 lines · Jan Beulich

Quoted 23 lines
Quoted 23 lines
Quoted 2 lines · On 21.05.15 at 13:48, <[email protected]> wrote:
Quoted 1 line · On 21.05.15 at 13:08, <[email protected]> wrote:
Quoted 17 lines · Jan Beulich
This is needed as stacks are going to become non-executable. Use separate stub pages (shared among suitable CPUs on the same node) instead.
Stub areas (currently 128 bytes each) are being split into two parts - a fixed usage one (the syscall ones) and dynamically usable space, which will be used by subsequent changes to hold dynamically generated code during instruction eumlation.
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.
Quoted 1 line
Signed-off-by Jan Beulich <[email protected]>
Reviewed-by Andrew Cooper <[email protected]>

Quoted 7 lines

Quoted 4 lines
Quoted 3 lines
It might be wise to have a BUILD_BUG_ON() which confirms that STUBS_PER_PAGE is a power of two, which is a requirement given the way it is used.
Good idea.
Sadly this can't be a BUILD_BUG_ON(), as STUBS_PER_PAGE isn't a compile time constant (due to the use of max()). I made it an ASSERT() for the time being.

In some copious free time, I shall see about borrowing some of the Linux constants infrastructure.

We have quite a few examples of calculations which should be able to be evaluated by the compiler, but cant given our current setup.

~Andrew

Jan Beulich

author

2015-05-21 10:16 UTC #8 lore raw

PATCH v2 2/4 x86emul: move stubs off the stack

In reply to Jan Beulich

This is needed as stacks are going to become non-executable.

Signed-off-by Jan Beulich <[email protected]>

---
v2: Limit the testing tool's stub buffer size to MAX_INST_LEN + 1. Fix
formatting.

tools/tests/x86_emulator/x86_emulate.c +4 -0

@@ -17,4 +17,8 @@ typedef bool bool_t;
 #define __packed __attribute__((packed))
 
 #include "x86_emulate/x86_emulate.h"
+
+#define get_stub(stb) ((void *)((stb).addr = (uintptr_t)(stb).buf))
+#define put_stub(stb)
+
 #include "x86_emulate/x86_emulate.c"

xen/arch/x86/x86_emulate.c +15 -0

@@ -9,6 +9,7 @@
  *    Keir Fraser <[email protected]>
  */
 
+#include <xen/domain_page.h>
 #include <asm/x86_emulate.h>
 #include <asm/asm_defns.h> /* mark_regs_dirty() */
 #include <asm/processor.h> /* current_cpu_info */

@@ -17,8 +18,22 @@
 /* Avoid namespace pollution. */
 #undef cmpxchg
 #undef cpuid
+#undef wbinvd
 
 #define cpu_has_amd_erratum(nr) \
         cpu_has_amd_erratum(&current_cpu_data, AMD_ERRATUM_##nr)
 
+#define get_stub(stb) ({                                   \
+    (stb).addr = this_cpu(stubs.addr) + STUB_BUF_SIZE / 2; \
+    ((stb).ptr = map_domain_page(this_cpu(stubs.mfn))) +   \
+        ((stb).addr & ~PAGE_MASK);                         \
+})
+#define put_stub(stb) ({                                   \
+    if ( (stb).ptr )                                       \
+    {                                                      \
+        unmap_domain_page((stb).ptr);                      \
+        (stb).ptr = NULL;                                  \
+    }                                                      \
+})
+
 #include "x86_emulate/x86_emulate.c"

xen/arch/x86/x86_emulate/x86_emulate.c +36 ~~-15~~

126 lines not on this page · raw message

xen/arch/x86/x86_emulate/x86_emulate.h +13 -0

@@ -429,6 +429,19 @@ struct x86_emulate_ctxt
     } retire;
 };
 
+struct x86_emulate_stub {
+    union {
+        void (*func)(void);
+        uintptr_t addr;
+    };
+#ifdef __XEN__
+    void *ptr;
+#else
+    /* Room for one insn and a (single byte) RET. */
+    uint8_t buf[MAX_INST_LEN + 1];
+#endif
+};
+
 /*
  * x86_emulate: Emulate an instruction.
  * Returns -1 on failure, 0 on success.

x86emul: move stubs off the stack

This is needed as stacks are going to become non-executable.

Signed-off-by Jan Beulich <[email protected]>

---
v2: Limit the testing tool's stub buffer size to MAX_INST_LEN + 1. Fix
formatting.

tools/tests/x86_emulator/x86_emulate.c +4 -0

@@ -17,4 +17,8 @@ typedef bool bool_t;
 #define __packed __attribute__((packed))
 
 #include "x86_emulate/x86_emulate.h"
+
+#define get_stub(stb) ((void *)((stb).addr = (uintptr_t)(stb).buf))
+#define put_stub(stb)
+
 #include "x86_emulate/x86_emulate.c"

xen/arch/x86/x86_emulate.c +15 -0

@@ -9,6 +9,7 @@
  *    Keir Fraser <[email protected]>
  */
 
+#include <xen/domain_page.h>
 #include <asm/x86_emulate.h>
 #include <asm/asm_defns.h> /* mark_regs_dirty() */
 #include <asm/processor.h> /* current_cpu_info */

@@ -17,8 +18,22 @@
 /* Avoid namespace pollution. */
 #undef cmpxchg
 #undef cpuid
+#undef wbinvd
 
 #define cpu_has_amd_erratum(nr) \
         cpu_has_amd_erratum(&current_cpu_data, AMD_ERRATUM_##nr)
 
+#define get_stub(stb) ({                                   \
+    (stb).addr = this_cpu(stubs.addr) + STUB_BUF_SIZE / 2; \
+    ((stb).ptr = map_domain_page(this_cpu(stubs.mfn))) +   \
+        ((stb).addr & ~PAGE_MASK);                         \
+})
+#define put_stub(stb) ({                                   \
+    if ( (stb).ptr )                                       \
+    {                                                      \
+        unmap_domain_page((stb).ptr);                      \
+        (stb).ptr = NULL;                                  \
+    }                                                      \
+})
+
 #include "x86_emulate/x86_emulate.c"

xen/arch/x86/x86_emulate/x86_emulate.c +36 ~~-15~~

126 lines not on this page · raw message

xen/arch/x86/x86_emulate/x86_emulate.h +13 -0

@@ -429,6 +429,19 @@ struct x86_emulate_ctxt
     } retire;
 };
 
+struct x86_emulate_stub {
+    union {
+        void (*func)(void);
+        uintptr_t addr;
+    };
+#ifdef __XEN__
+    void *ptr;
+#else
+    /* Room for one insn and a (single byte) RET. */
+    uint8_t buf[MAX_INST_LEN + 1];
+#endif
+};
+
 /*
  * x86_emulate: Emulate an instruction.
  * Returns -1 on failure, 0 on success.

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Andrew Cooper

Reviewed-by on v2 2/4

2015-05-21 11:10 UTC #9 lore raw

Re: [PATCH v2 2/4] x86emul: move stubs off the stack

Quoted 4 lines · Jan Beulich

This is needed as stacks are going to become non-executable.
Signed-off-by Jan Beulich <[email protected]>

Reviewed-by Andrew Cooper <[email protected]>

Jan Beulich

author

2015-05-21 10:17 UTC #10 lore raw

PATCH v2 3/4 x86: move I/O emulation stubs off the stack

In reply to Jan Beulich

This is needed as stacks are going to become non-executable.

Signed-off-by Jan Beulich <[email protected]>

---
v2: Use ~PAGE_MASK instead of PAGE_SIZE - 1 for page offset masking.
Comment adjustments.

xen/arch/x86/traps.c +11 -4

@@ -2006,7 +2006,7 @@ static int emulate_privileged_op(struct 
                            ? (*(u32 *)&regs->reg = (val)) \
                            : (*(u16 *)&regs->reg = (val)))
     unsigned long code_base, code_limit;
-    char io_emul_stub[32];
+    char *io_emul_stub = NULL;
     void (*io_emul)(struct cpu_user_regs *) __attribute__((__regparm__(1)));
     uint64_t val;

@@ -2191,10 +2191,13 @@ static int emulate_privileged_op(struct 
 
     /*
      * Very likely to be an I/O instruction (IN/OUT).
-     * Build an on-stack stub to execute the instruction with full guest
-     * GPR context. This is needed for some systems which (ab)use IN/OUT
+     * Build an stub to execute the instruction with full guest GPR
+     * context. This is needed for some systems which (ab)use IN/OUT
      * to communicate with BIOS code in system-management mode.
      */
+    io_emul_stub = map_domain_page(this_cpu(stubs.mfn)) +
+                   (this_cpu(stubs.addr) & ~PAGE_MASK) +
+                   STUB_BUF_SIZE / 2;
     /* movq $host_to_guest_gpr_switch,%rcx */
     io_emul_stub[0] = 0x48;
     io_emul_stub[1] = 0xb9;

@@ -2212,7 +2215,7 @@ static int emulate_privileged_op(struct 
     io_emul_stub[15] = 0xc3;
 
     /* Handy function-typed pointer to the stub. */
-    io_emul = (void *)io_emul_stub;
+    io_emul = (void *)(this_cpu(stubs.addr) + STUB_BUF_SIZE / 2);
 
     if ( ioemul_handle_quirk )
         ioemul_handle_quirk(opcode, &io_emul_stub[12], regs);

@@ -2777,9 +2780,13 @@ static int emulate_privileged_op(struct 
  done:
     instruction_done(regs, eip, bpmatch);
  skip:
+    if ( io_emul_stub )
+        unmap_domain_page(io_emul_stub);
     return EXCRET_fault_fixed;
 
  fail:
+    if ( io_emul_stub )
+        unmap_domain_page(io_emul_stub);
     return 0;
 }

x86: move I/O emulation stubs off the stack

This is needed as stacks are going to become non-executable.

Signed-off-by Jan Beulich <[email protected]>

---
v2: Use ~PAGE_MASK instead of PAGE_SIZE - 1 for page offset masking.
Comment adjustments.

xen/arch/x86/traps.c +11 -4

@@ -2006,7 +2006,7 @@ static int emulate_privileged_op(struct 
                            ? (*(u32 *)&regs->reg = (val)) \
                            : (*(u16 *)&regs->reg = (val)))
     unsigned long code_base, code_limit;
-    char io_emul_stub[32];
+    char *io_emul_stub = NULL;
     void (*io_emul)(struct cpu_user_regs *) __attribute__((__regparm__(1)));
     uint64_t val;

@@ -2191,10 +2191,13 @@ static int emulate_privileged_op(struct 
 
     /*
      * Very likely to be an I/O instruction (IN/OUT).
-     * Build an on-stack stub to execute the instruction with full guest
-     * GPR context. This is needed for some systems which (ab)use IN/OUT
+     * Build an stub to execute the instruction with full guest GPR
+     * context. This is needed for some systems which (ab)use IN/OUT
      * to communicate with BIOS code in system-management mode.
      */
+    io_emul_stub = map_domain_page(this_cpu(stubs.mfn)) +
+                   (this_cpu(stubs.addr) & ~PAGE_MASK) +
+                   STUB_BUF_SIZE / 2;
     /* movq $host_to_guest_gpr_switch,%rcx */
     io_emul_stub[0] = 0x48;
     io_emul_stub[1] = 0xb9;

@@ -2212,7 +2215,7 @@ static int emulate_privileged_op(struct 
     io_emul_stub[15] = 0xc3;
 
     /* Handy function-typed pointer to the stub. */
-    io_emul = (void *)io_emul_stub;
+    io_emul = (void *)(this_cpu(stubs.addr) + STUB_BUF_SIZE / 2);
 
     if ( ioemul_handle_quirk )
         ioemul_handle_quirk(opcode, &io_emul_stub[12], regs);

@@ -2777,9 +2780,13 @@ static int emulate_privileged_op(struct 
  done:
     instruction_done(regs, eip, bpmatch);
  skip:
+    if ( io_emul_stub )
+        unmap_domain_page(io_emul_stub);
     return EXCRET_fault_fixed;
 
  fail:
+    if ( io_emul_stub )
+        unmap_domain_page(io_emul_stub);
     return 0;
 }

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Andrew Cooper

Reviewed-by on v2 3/4

2015-05-21 11:12 UTC #11 lore raw

Re: [PATCH v2 3/4] x86: move I/O emulation stubs off the stack

Quoted 4 lines · Jan Beulich

This is needed as stacks are going to become non-executable.
Signed-off-by Jan Beulich <[email protected]>

Reviewed-by Andrew Cooper <[email protected]>

Jan Beulich

author

2015-05-21 10:17 UTC #12 lore raw

PATCH v2 4/4 x86: switch default mapping attributes to non-executable

In reply to Jan Beulich

Only a very limited subset of mappings need to be done as executable ones; in particular the direct mapping should not be executable to limit the damage attackers can cause by exploiting security relevant bugs.

The EFI change at once includes an adjustment to set NX only when supported by the hardware.

Signed-off-by Jan Beulich <[email protected]>

---
v2: Log a message indicating NX protection state.

xen/arch/x86/domain.c +1 -1

@@ -293,7 +293,7 @@ struct vcpu_guest_context *alloc_vcpu_gu
             free_vcpu_guest_context(NULL);
             return NULL;
         }
-        __set_fixmap(idx - i, page_to_mfn(pg), __PAGE_HYPERVISOR);
+        __set_fixmap(idx - i, page_to_mfn(pg), __PAGE_HYPERVISOR_RW);
         per_cpu(vgc_pages[i], cpu) = pg;
     }
     return (void *)fix_to_virt(idx);

xen/arch/x86/domain_page.c +1 -1

@@ -160,7 +160,7 @@ void *map_domain_page(unsigned long mfn)
 
     spin_unlock(&dcache->lock);
 
-    l1e_write(&MAPCACHE_L1ENT(idx), l1e_from_pfn(mfn, __PAGE_HYPERVISOR));
+    l1e_write(&MAPCACHE_L1ENT(idx), l1e_from_pfn(mfn, __PAGE_HYPERVISOR_RW));
 
  out:
     local_irq_restore(flags);

xen/arch/x86/mm.c +4 -4

@@ -4416,7 +4416,7 @@ long set_gdt(struct vcpu *v, 
     for ( i = 0; i < nr_pages; i++ )
     {
         v->arch.pv_vcpu.gdt_frames[i] = frames[i];
-        l1e_write(&pl1e[i], l1e_from_pfn(frames[i], __PAGE_HYPERVISOR));
+        l1e_write(&pl1e[i], l1e_from_pfn(frames[i], __PAGE_HYPERVISOR_RW));
     }
 
     xfree(pfns);

@@ -6007,7 +6007,7 @@ int create_perdomain_mapping(struct doma
                 if ( !IS_NIL(ppg) )
                     *ppg++ = pg;
                 l1tab[l1_table_offset(va)] =
-                    l1e_from_page(pg, __PAGE_HYPERVISOR | _PAGE_AVAIL0);
+                    l1e_from_page(pg, __PAGE_HYPERVISOR_RW | _PAGE_AVAIL0);
                 l2e_add_flags(*pl2e, _PAGE_AVAIL0);
             }
             else

@@ -6136,7 +6136,7 @@ void memguard_init(void)
         (unsigned long)__va(start),
         start >> PAGE_SHIFT,
         (__pa(&_end) + PAGE_SIZE - 1 - start) >> PAGE_SHIFT,
-        __PAGE_HYPERVISOR|MAP_SMALL_PAGES);
+        __PAGE_HYPERVISOR_RW|MAP_SMALL_PAGES);
     BUG_ON(start != xen_phys_start);
     map_pages_to_xen(
         XEN_VIRT_START,

@@ -6149,7 +6149,7 @@ static void __memguard_change_range(void
 {
     unsigned long _p = (unsigned long)p;
     unsigned long _l = (unsigned long)l;
-    unsigned int flags = __PAGE_HYPERVISOR | MAP_SMALL_PAGES;
+    unsigned int flags = __PAGE_HYPERVISOR_RW | MAP_SMALL_PAGES;
 
     /* Ensure we are dealing with a page-aligned whole number of pages. */
     ASSERT((_p&~PAGE_MASK) == 0);

xen/arch/x86/setup.c +6 -2

@@ -900,7 +900,7 @@ void __init noreturn __start_xen(unsigne
             /* The only data mappings to be relocated are in the Xen area. */
             pl2e = __va(__pa(l2_xenmap));
             *pl2e++ = l2e_from_pfn(xen_phys_start >> PAGE_SHIFT,
-                                   PAGE_HYPERVISOR | _PAGE_PSE);
+                                   PAGE_HYPERVISOR_RWX | _PAGE_PSE);
             for ( i = 1; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ )
             {
                 if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) )

@@ -1087,7 +1087,7 @@ void __init noreturn __start_xen(unsigne
             /* This range must not be passed to the boot allocator and
              * must also not be mapped with _PAGE_GLOBAL. */
             map_pages_to_xen((unsigned long)__va(map_e), PFN_DOWN(map_e),
-                             PFN_DOWN(e - map_e), __PAGE_HYPERVISOR);
+                             PFN_DOWN(e - map_e), __PAGE_HYPERVISOR_RW);
         }
         if ( s < map_s )
         {

@@ -1423,6 +1423,10 @@ void __init noreturn __start_xen(unsigne
     if ( cpu_has_smap )
         write_cr4(read_cr4() & ~X86_CR4_SMAP);
 
+    printk("%sNX (Execute Disable) protection %sactive\n",
+           cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
+           cpu_has_nx ? "" : "not ");
+
     /*
      * We're going to setup domain0 using the module(s) that we stashed safely
      * above our heap. The second module, if present, is an initrd ramdisk.

xen/arch/x86/x86_64/mm.c +28 -1

@@ -895,6 +895,33 @@ void __init subarch_init_memory(void)
             share_xen_page_with_privileged_guests(page, XENSHARE_readonly);
         }
     }
+
+    /* Mark low 16Mb of direct map NX if hardware supports it. */
+    if ( !cpu_has_nx )
+        return;
+
+    v = DIRECTMAP_VIRT_START + (1UL << 20);
+    l3e = l4e_to_l3e(idle_pg_table[l4_table_offset(v)])[l3_table_offset(v)];
+    ASSERT(l3e_get_flags(l3e) & _PAGE_PRESENT);
+    do {
+        l2e = l3e_to_l2e(l3e)[l2_table_offset(v)];
+        ASSERT(l2e_get_flags(l2e) & _PAGE_PRESENT);
+        if ( l2e_get_flags(l2e) & _PAGE_PSE )
+        {
+            l2e_add_flags(l2e, _PAGE_NX_BIT);
+            l3e_to_l2e(l3e)[l2_table_offset(v)] = l2e;
+            v += 1 << L2_PAGETABLE_SHIFT;
+        }
+        else
+        {
+            l1_pgentry_t l1e = l2e_to_l1e(l2e)[l1_table_offset(v)];
+
+            ASSERT(l1e_get_flags(l1e) & _PAGE_PRESENT);
+            l1e_add_flags(l1e, _PAGE_NX_BIT);
+            l2e_to_l1e(l2e)[l1_table_offset(v)] = l1e;
+            v += 1 << L1_PAGETABLE_SHIFT;
+        }
+    } while ( v < DIRECTMAP_VIRT_START + (16UL << 20) );
 }
 
 long subarch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)

@@ -1359,7 +1386,7 @@ int memory_add(unsigned long spfn, unsig
         if ( i < spfn )
             i = spfn;
         ret = map_pages_to_xen((unsigned long)mfn_to_virt(i), i,
-                               epfn - i, __PAGE_HYPERVISOR);
+                               epfn - i, __PAGE_HYPERVISOR_RW);
         if ( ret )
             return ret;
     }

xen/common/efi/boot.c +2 -2

@@ -1162,7 +1162,7 @@ void __init efi_init_memory(void)
         EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
         u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
         unsigned long smfn, emfn;
-        unsigned int prot = PAGE_HYPERVISOR;
+        unsigned int prot = PAGE_HYPERVISOR_RWX;
 
         printk(XENLOG_INFO " %013" PRIx64 "-%013" PRIx64
                            " type=%u attr=%016" PRIx64 "\n",

@@ -1195,7 +1195,7 @@ void __init efi_init_memory(void)
         if ( desc->Attribute & EFI_MEMORY_WP )
             prot &= _PAGE_RW;
         if ( desc->Attribute & EFI_MEMORY_XP )
-            prot |= _PAGE_NX_BIT;
+            prot |= _PAGE_NX;
 
         if ( pfn_to_pdx(emfn - 1) < (DIRECTMAP_SIZE >> PAGE_SHIFT) &&
              !(smfn & pfn_hole_mask) &&

xen/include/asm-x86/page.h +5 -1

@@ -304,7 +304,8 @@ void efi_update_l4_pgtable(unsigned int 
 #define _PAGE_AVAIL1   _AC(0x400,U)
 #define _PAGE_AVAIL2   _AC(0x800,U)
 #define _PAGE_AVAIL    _AC(0xE00,U)
-#define _PAGE_PSE_PAT _AC(0x1000,U)
+#define _PAGE_PSE_PAT  _AC(0x1000,U)
+#define _PAGE_NX       (cpu_has_nx ? _PAGE_NX_BIT : 0)
 /* non-architectural flags */
 #define _PAGE_PAGED   0x2000U
 #define _PAGE_SHARED  0x4000U

@@ -321,6 +322,9 @@ void efi_update_l4_pgtable(unsigned int 
 #define _PAGE_GNTTAB   0
 #endif
 
+#define __PAGE_HYPERVISOR_RO      (_PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_NX)
+#define __PAGE_HYPERVISOR_RW      (__PAGE_HYPERVISOR_RO | \
+                                   _PAGE_DIRTY | _PAGE_RW)
 #define __PAGE_HYPERVISOR_RX      (_PAGE_PRESENT | _PAGE_ACCESSED)
 #define __PAGE_HYPERVISOR         (__PAGE_HYPERVISOR_RX | \
                                    _PAGE_DIRTY | _PAGE_RW)

xen/include/asm-x86/x86_64/page.h +13 -2

@@ -147,9 +147,20 @@ typedef l4_pgentry_t root_pgentry_t;
  */
 #define _PAGE_GUEST_KERNEL (1U<<12)
 
-#define PAGE_HYPERVISOR         (__PAGE_HYPERVISOR         | _PAGE_GLOBAL)
+#define PAGE_HYPERVISOR_RO      (__PAGE_HYPERVISOR_RO      | _PAGE_GLOBAL)
+#define PAGE_HYPERVISOR_RW      (__PAGE_HYPERVISOR_RW      | _PAGE_GLOBAL)
 #define PAGE_HYPERVISOR_RX      (__PAGE_HYPERVISOR_RX      | _PAGE_GLOBAL)
-#define PAGE_HYPERVISOR_NOCACHE (__PAGE_HYPERVISOR_NOCACHE | _PAGE_GLOBAL)
+#define PAGE_HYPERVISOR_RWX     (__PAGE_HYPERVISOR         | _PAGE_GLOBAL)
+
+#ifdef __ASSEMBLY__
+/* Dependency on NX being available can't be expressed. */
+# define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RWX
+# define PAGE_HYPERVISOR_NOCACHE (__PAGE_HYPERVISOR_NOCACHE | _PAGE_GLOBAL)
+#else
+# define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RW
+# define PAGE_HYPERVISOR_NOCACHE (__PAGE_HYPERVISOR_NOCACHE | \
+                                  _PAGE_GLOBAL | _PAGE_NX)
+#endif
 
 #endif /* __X86_64_PAGE_H__ */

x86: switch default mapping attributes to non-executable

The EFI change at once includes an adjustment to set NX only when supported by the hardware.

Signed-off-by Jan Beulich <[email protected]>

---
v2: Log a message indicating NX protection state.

xen/arch/x86/domain.c +1 -1

@@ -293,7 +293,7 @@ struct vcpu_guest_context *alloc_vcpu_gu
             free_vcpu_guest_context(NULL);
             return NULL;
         }
-        __set_fixmap(idx - i, page_to_mfn(pg), __PAGE_HYPERVISOR);
+        __set_fixmap(idx - i, page_to_mfn(pg), __PAGE_HYPERVISOR_RW);
         per_cpu(vgc_pages[i], cpu) = pg;
     }
     return (void *)fix_to_virt(idx);

xen/arch/x86/domain_page.c +1 -1

@@ -160,7 +160,7 @@ void *map_domain_page(unsigned long mfn)
 
     spin_unlock(&dcache->lock);
 
-    l1e_write(&MAPCACHE_L1ENT(idx), l1e_from_pfn(mfn, __PAGE_HYPERVISOR));
+    l1e_write(&MAPCACHE_L1ENT(idx), l1e_from_pfn(mfn, __PAGE_HYPERVISOR_RW));
 
  out:
     local_irq_restore(flags);

xen/arch/x86/mm.c +4 -4

@@ -4416,7 +4416,7 @@ long set_gdt(struct vcpu *v, 
     for ( i = 0; i < nr_pages; i++ )
     {
         v->arch.pv_vcpu.gdt_frames[i] = frames[i];
-        l1e_write(&pl1e[i], l1e_from_pfn(frames[i], __PAGE_HYPERVISOR));
+        l1e_write(&pl1e[i], l1e_from_pfn(frames[i], __PAGE_HYPERVISOR_RW));
     }
 
     xfree(pfns);

@@ -6007,7 +6007,7 @@ int create_perdomain_mapping(struct doma
                 if ( !IS_NIL(ppg) )
                     *ppg++ = pg;
                 l1tab[l1_table_offset(va)] =
-                    l1e_from_page(pg, __PAGE_HYPERVISOR | _PAGE_AVAIL0);
+                    l1e_from_page(pg, __PAGE_HYPERVISOR_RW | _PAGE_AVAIL0);
                 l2e_add_flags(*pl2e, _PAGE_AVAIL0);
             }
             else

@@ -6136,7 +6136,7 @@ void memguard_init(void)
         (unsigned long)__va(start),
         start >> PAGE_SHIFT,
         (__pa(&_end) + PAGE_SIZE - 1 - start) >> PAGE_SHIFT,
-        __PAGE_HYPERVISOR|MAP_SMALL_PAGES);
+        __PAGE_HYPERVISOR_RW|MAP_SMALL_PAGES);
     BUG_ON(start != xen_phys_start);
     map_pages_to_xen(
         XEN_VIRT_START,

@@ -6149,7 +6149,7 @@ static void __memguard_change_range(void
 {
     unsigned long _p = (unsigned long)p;
     unsigned long _l = (unsigned long)l;
-    unsigned int flags = __PAGE_HYPERVISOR | MAP_SMALL_PAGES;
+    unsigned int flags = __PAGE_HYPERVISOR_RW | MAP_SMALL_PAGES;
 
     /* Ensure we are dealing with a page-aligned whole number of pages. */
     ASSERT((_p&~PAGE_MASK) == 0);

xen/arch/x86/setup.c +6 -2

@@ -900,7 +900,7 @@ void __init noreturn __start_xen(unsigne
             /* The only data mappings to be relocated are in the Xen area. */
             pl2e = __va(__pa(l2_xenmap));
             *pl2e++ = l2e_from_pfn(xen_phys_start >> PAGE_SHIFT,
-                                   PAGE_HYPERVISOR | _PAGE_PSE);
+                                   PAGE_HYPERVISOR_RWX | _PAGE_PSE);
             for ( i = 1; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ )
             {
                 if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) )

@@ -1087,7 +1087,7 @@ void __init noreturn __start_xen(unsigne
             /* This range must not be passed to the boot allocator and
              * must also not be mapped with _PAGE_GLOBAL. */
             map_pages_to_xen((unsigned long)__va(map_e), PFN_DOWN(map_e),
-                             PFN_DOWN(e - map_e), __PAGE_HYPERVISOR);
+                             PFN_DOWN(e - map_e), __PAGE_HYPERVISOR_RW);
         }
         if ( s < map_s )
         {

@@ -1423,6 +1423,10 @@ void __init noreturn __start_xen(unsigne
     if ( cpu_has_smap )
         write_cr4(read_cr4() & ~X86_CR4_SMAP);
 
+    printk("%sNX (Execute Disable) protection %sactive\n",
+           cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
+           cpu_has_nx ? "" : "not ");
+
     /*
      * We're going to setup domain0 using the module(s) that we stashed safely
      * above our heap. The second module, if present, is an initrd ramdisk.

xen/arch/x86/x86_64/mm.c +28 -1

@@ -895,6 +895,33 @@ void __init subarch_init_memory(void)
             share_xen_page_with_privileged_guests(page, XENSHARE_readonly);
         }
     }
+
+    /* Mark low 16Mb of direct map NX if hardware supports it. */
+    if ( !cpu_has_nx )
+        return;
+
+    v = DIRECTMAP_VIRT_START + (1UL << 20);
+    l3e = l4e_to_l3e(idle_pg_table[l4_table_offset(v)])[l3_table_offset(v)];
+    ASSERT(l3e_get_flags(l3e) & _PAGE_PRESENT);
+    do {
+        l2e = l3e_to_l2e(l3e)[l2_table_offset(v)];
+        ASSERT(l2e_get_flags(l2e) & _PAGE_PRESENT);
+        if ( l2e_get_flags(l2e) & _PAGE_PSE )
+        {
+            l2e_add_flags(l2e, _PAGE_NX_BIT);
+            l3e_to_l2e(l3e)[l2_table_offset(v)] = l2e;
+            v += 1 << L2_PAGETABLE_SHIFT;
+        }
+        else
+        {
+            l1_pgentry_t l1e = l2e_to_l1e(l2e)[l1_table_offset(v)];
+
+            ASSERT(l1e_get_flags(l1e) & _PAGE_PRESENT);
+            l1e_add_flags(l1e, _PAGE_NX_BIT);
+            l2e_to_l1e(l2e)[l1_table_offset(v)] = l1e;
+            v += 1 << L1_PAGETABLE_SHIFT;
+        }
+    } while ( v < DIRECTMAP_VIRT_START + (16UL << 20) );
 }
 
 long subarch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)

@@ -1359,7 +1386,7 @@ int memory_add(unsigned long spfn, unsig
         if ( i < spfn )
             i = spfn;
         ret = map_pages_to_xen((unsigned long)mfn_to_virt(i), i,
-                               epfn - i, __PAGE_HYPERVISOR);
+                               epfn - i, __PAGE_HYPERVISOR_RW);
         if ( ret )
             return ret;
     }

xen/common/efi/boot.c +2 -2

@@ -1162,7 +1162,7 @@ void __init efi_init_memory(void)
         EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
         u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
         unsigned long smfn, emfn;
-        unsigned int prot = PAGE_HYPERVISOR;
+        unsigned int prot = PAGE_HYPERVISOR_RWX;
 
         printk(XENLOG_INFO " %013" PRIx64 "-%013" PRIx64
                            " type=%u attr=%016" PRIx64 "\n",

@@ -1195,7 +1195,7 @@ void __init efi_init_memory(void)
         if ( desc->Attribute & EFI_MEMORY_WP )
             prot &= _PAGE_RW;
         if ( desc->Attribute & EFI_MEMORY_XP )
-            prot |= _PAGE_NX_BIT;
+            prot |= _PAGE_NX;
 
         if ( pfn_to_pdx(emfn - 1) < (DIRECTMAP_SIZE >> PAGE_SHIFT) &&
              !(smfn & pfn_hole_mask) &&

xen/include/asm-x86/page.h +5 -1

@@ -304,7 +304,8 @@ void efi_update_l4_pgtable(unsigned int 
 #define _PAGE_AVAIL1   _AC(0x400,U)
 #define _PAGE_AVAIL2   _AC(0x800,U)
 #define _PAGE_AVAIL    _AC(0xE00,U)
-#define _PAGE_PSE_PAT _AC(0x1000,U)
+#define _PAGE_PSE_PAT  _AC(0x1000,U)
+#define _PAGE_NX       (cpu_has_nx ? _PAGE_NX_BIT : 0)
 /* non-architectural flags */
 #define _PAGE_PAGED   0x2000U
 #define _PAGE_SHARED  0x4000U

@@ -321,6 +322,9 @@ void efi_update_l4_pgtable(unsigned int 
 #define _PAGE_GNTTAB   0
 #endif
 
+#define __PAGE_HYPERVISOR_RO      (_PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_NX)
+#define __PAGE_HYPERVISOR_RW      (__PAGE_HYPERVISOR_RO | \
+                                   _PAGE_DIRTY | _PAGE_RW)
 #define __PAGE_HYPERVISOR_RX      (_PAGE_PRESENT | _PAGE_ACCESSED)
 #define __PAGE_HYPERVISOR         (__PAGE_HYPERVISOR_RX | \
                                    _PAGE_DIRTY | _PAGE_RW)

xen/include/asm-x86/x86_64/page.h +13 -2

@@ -147,9 +147,20 @@ typedef l4_pgentry_t root_pgentry_t;
  */
 #define _PAGE_GUEST_KERNEL (1U<<12)
 
-#define PAGE_HYPERVISOR         (__PAGE_HYPERVISOR         | _PAGE_GLOBAL)
+#define PAGE_HYPERVISOR_RO      (__PAGE_HYPERVISOR_RO      | _PAGE_GLOBAL)
+#define PAGE_HYPERVISOR_RW      (__PAGE_HYPERVISOR_RW      | _PAGE_GLOBAL)
 #define PAGE_HYPERVISOR_RX      (__PAGE_HYPERVISOR_RX      | _PAGE_GLOBAL)
-#define PAGE_HYPERVISOR_NOCACHE (__PAGE_HYPERVISOR_NOCACHE | _PAGE_GLOBAL)
+#define PAGE_HYPERVISOR_RWX     (__PAGE_HYPERVISOR         | _PAGE_GLOBAL)
+
+#ifdef __ASSEMBLY__
+/* Dependency on NX being available can't be expressed. */
+# define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RWX
+# define PAGE_HYPERVISOR_NOCACHE (__PAGE_HYPERVISOR_NOCACHE | _PAGE_GLOBAL)
+#else
+# define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RW
+# define PAGE_HYPERVISOR_NOCACHE (__PAGE_HYPERVISOR_NOCACHE | \
+                                  _PAGE_GLOBAL | _PAGE_NX)
+#endif
 
 #endif /* __X86_64_PAGE_H__ */

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Andrew Cooper

Reviewed-by on v2 4/4

2015-05-21 11:14 UTC #13 lore raw

Re: [PATCH v2 4/4] x86: switch default mapping attributes to non-executable

Quoted 9 lines · Jan Beulich

Only a very limited subset of mappings need to be done as executable ones; in particular the direct mapping should not be executable to limit the damage attackers can cause by exploiting security relevant bugs.
The EFI change at once includes an adjustment to set NX only when supported by the hardware.
Signed-off-by Jan Beulich <[email protected]>

Reviewed-by Andrew Cooper <[email protected]>