|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [PATCH v2 3/8] x86/PV: harden guest memory accesses against speculative abuse
Inspired by https://lore.kernel.org/lkml/f12e7d3cecf41b2c29734ea45a393be21d4a8058.1597848273.git.jpoimboe@xxxxxxxxxx/ and prior work in that area of x86 Linux, suppress speculation with guest specified pointer values by suitably masking the addresses to non-canonical space in case they fall into Xen's virtual address range. Introduce a new Kconfig control. Note that it is necessary in such code to avoid using "m" kind operands: If we didn't, there would be no guarantee that the register passed to guest_access_mask_ptr is also the (base) one used for the memory access. As a minor unrelated change in get_unsafe_asm() the unnecessary "itype" parameter gets dropped and the XOR on the fixup path gets changed to be a 32-bit one in all cases: This way we avoid pointless REX.W or operand size overrides, or writes to partial registers. Requested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx> --- v2: Add comment to assembler macro. --- The insn sequence chosen is certainly up for discussion; I've picked this one despite the RCR because alternatives I could come up with, like mov $(HYPERVISOR_VIRT_END), %rax mov $~0, %rdx mov $0x7fffffffffffffff, %rcx cmp %rax, %rdi cmovb %rcx, %rdx and %rdx, %rdi weren't necessarily better: Either, as above, they are longer and require a 3rd scratch register, or they also utilize the carry flag in some similar way. --- Judging from the comment ahead of put_unsafe_asm() we might as well not tell gcc at all anymore about the memory access there, now that there's no use of the operand anymore in the assembly code. --- a/xen/arch/x86/usercopy.c +++ b/xen/arch/x86/usercopy.c @@ -10,12 +10,19 @@ #include <xen/sched.h> #include <asm/uaccess.h> -unsigned __copy_to_user_ll(void __user *to, const void *from, unsigned n) +#ifndef GUARD +# define GUARD UA_KEEP +#endif + +unsigned int copy_to_guest_ll(void __user *to, const void *from, unsigned int n) { unsigned dummy; stac(); asm volatile ( + GUARD( + " guest_access_mask_ptr %[to], %q[scratch1], %q[scratch2]\n" + ) " cmp $"STR(2*BYTES_PER_LONG-1)", %[cnt]\n" " jbe 1f\n" " mov %k[to], %[cnt]\n" @@ -42,6 +49,7 @@ unsigned __copy_to_user_ll(void __user * _ASM_EXTABLE(1b, 2b) : [cnt] "+c" (n), [to] "+D" (to), [from] "+S" (from), [aux] "=&r" (dummy) + GUARD(, [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy)) : "[aux]" (n) : "memory" ); clac(); @@ -49,12 +57,15 @@ unsigned __copy_to_user_ll(void __user * return n; } -unsigned __copy_from_user_ll(void *to, const void __user *from, unsigned n) +unsigned int copy_from_guest_ll(void *to, const void __user *from, unsigned int n) { unsigned dummy; stac(); asm volatile ( + GUARD( + " guest_access_mask_ptr %[from], %q[scratch1], %q[scratch2]\n" + ) " cmp $"STR(2*BYTES_PER_LONG-1)", %[cnt]\n" " jbe 1f\n" " mov %k[to], %[cnt]\n" @@ -87,6 +98,7 @@ unsigned __copy_from_user_ll(void *to, c _ASM_EXTABLE(1b, 6b) : [cnt] "+c" (n), [to] "+D" (to), [from] "+S" (from), [aux] "=&r" (dummy) + GUARD(, [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy)) : "[aux]" (n) : "memory" ); clac(); @@ -94,6 +106,8 @@ unsigned __copy_from_user_ll(void *to, c return n; } +#if GUARD(1) + 0 + /** * copy_to_user: - Copy a block of data into user space. * @to: Destination address, in user space. @@ -128,8 +142,11 @@ unsigned clear_user(void __user *to, uns { if ( access_ok(to, n) ) { + long dummy; + stac(); asm volatile ( + " guest_access_mask_ptr %[to], %[scratch1], %[scratch2]\n" "0: rep stos"__OS"\n" " mov %[bytes], %[cnt]\n" "1: rep stosb\n" @@ -140,7 +157,8 @@ unsigned clear_user(void __user *to, uns ".previous\n" _ASM_EXTABLE(0b,3b) _ASM_EXTABLE(1b,2b) - : [cnt] "=&c" (n), [to] "+D" (to) + : [cnt] "=&c" (n), [to] "+D" (to), [scratch1] "=&r" (dummy), + [scratch2] "=&r" (dummy) : [bytes] "r" (n & (BYTES_PER_LONG - 1)), [longs] "0" (n / BYTES_PER_LONG), "a" (0) ); clac(); @@ -174,6 +192,16 @@ unsigned copy_from_user(void *to, const return n; } +# undef GUARD +# define GUARD UA_DROP +# define copy_to_guest_ll copy_to_unsafe_ll +# define copy_from_guest_ll copy_from_unsafe_ll +# undef __user +# define __user +# include __FILE__ + +#endif /* GUARD(1) */ + /* * Local variables: * mode: C --- a/xen/arch/x86/x86_64/entry.S +++ b/xen/arch/x86/x86_64/entry.S @@ -458,6 +458,8 @@ UNLIKELY_START(g, create_bounce_frame_ba jmp asm_domain_crash_synchronous /* Does not return */ __UNLIKELY_END(create_bounce_frame_bad_sp) + guest_access_mask_ptr %rsi, %rax, %rcx + #define STORE_GUEST_STACK(reg, n) \ 0: movq %reg,(n)*8(%rsi); \ _ASM_EXTABLE(0b, domain_crash_page_fault_ ## n ## x8) --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -111,6 +111,24 @@ config SPECULATIVE_HARDEN_BRANCH If unsure, say Y. +config SPECULATIVE_HARDEN_GUEST_ACCESS + bool "Speculative PV Guest Memory Access Hardening" + default y + depends on PV + help + Contemporary processors may use speculative execution as a + performance optimisation, but this can potentially be abused by an + attacker to leak data via speculative sidechannels. + + One source of data leakage is via speculative accesses to hypervisor + memory through guest controlled values used to access guest memory. + + When enabled, code paths accessing PV guest memory will have guest + controlled addresses massaged such that memory accesses through them + won't touch hypervisor address space. + + If unsure, say Y. + endmenu config HYPFS --- a/xen/include/asm-x86/asm-defns.h +++ b/xen/include/asm-x86/asm-defns.h @@ -56,3 +56,23 @@ .macro INDIRECT_JMP arg:req INDIRECT_BRANCH jmp \arg .endm + +.macro guest_access_mask_ptr ptr:req, scratch1:req, scratch2:req +#if defined(CONFIG_SPECULATIVE_HARDEN_GUEST_ACCESS) + /* + * Here we want + * + * ptr &= ~0ull >> (ptr < HYPERVISOR_VIRT_END); + * + * but guaranteed without any conditional branches (hence in assembly). + */ + mov $(HYPERVISOR_VIRT_END - 1), \scratch1 + mov $~0, \scratch2 + cmp \ptr, \scratch1 + rcr $1, \scratch2 + and \scratch2, \ptr +#elif defined(CONFIG_DEBUG) && defined(CONFIG_PV) + xor $~\@, \scratch1 + xor $~\@, \scratch2 +#endif +.endm --- a/xen/include/asm-x86/uaccess.h +++ b/xen/include/asm-x86/uaccess.h @@ -12,13 +12,19 @@ unsigned copy_to_user(void *to, const void *from, unsigned len); unsigned clear_user(void *to, unsigned len); unsigned copy_from_user(void *to, const void *from, unsigned len); + /* Handles exceptions in both to and from, but doesn't do access_ok */ -unsigned __copy_to_user_ll(void __user*to, const void *from, unsigned n); -unsigned __copy_from_user_ll(void *to, const void __user *from, unsigned n); +unsigned int copy_to_guest_ll(void __user*to, const void *from, unsigned int n); +unsigned int copy_from_guest_ll(void *to, const void __user *from, unsigned int n); +unsigned int copy_to_unsafe_ll(void *to, const void *from, unsigned int n); +unsigned int copy_from_unsafe_ll(void *to, const void *from, unsigned int n); extern long __get_user_bad(void); extern void __put_user_bad(void); +#define UA_KEEP(args...) args +#define UA_DROP(args...) + /** * get_user: - Get a simple variable from user space. * @x: Variable to store result. @@ -77,7 +83,6 @@ extern void __put_user_bad(void); * On error, the variable @x is set to zero. */ #define __get_guest(x, ptr) get_guest_nocheck(x, ptr, sizeof(*(ptr))) -#define get_unsafe __get_guest /** * __put_guest: - Write a simple value into guest space, with less checking. @@ -98,7 +103,13 @@ extern void __put_user_bad(void); */ #define __put_guest(x, ptr) \ put_guest_nocheck((__typeof__(*(ptr)))(x), ptr, sizeof(*(ptr))) -#define put_unsafe __put_guest + +#define put_unsafe(x, ptr) \ +({ \ + int err_; \ + put_unsafe_size(x, ptr, sizeof(*(ptr)), UA_DROP, err_, -EFAULT);\ + err_; \ +}) #define put_guest_nocheck(x, ptr, size) \ ({ \ @@ -115,6 +126,13 @@ extern void __put_user_bad(void); : -EFAULT; \ }) +#define get_unsafe(x, ptr) \ +({ \ + int err_; \ + get_unsafe_size(x, ptr, sizeof(*(ptr)), UA_DROP, err_, -EFAULT);\ + err_; \ +}) + #define get_guest_nocheck(x, ptr, size) \ ({ \ int err_; \ @@ -138,62 +156,87 @@ struct __large_struct { unsigned long bu * we do not write to any memory gcc knows about, so there are no * aliasing issues. */ -#define put_unsafe_asm(x, addr, err, itype, rtype, ltype, errret) \ +#define put_unsafe_asm(x, addr, GUARD, err, itype, rtype, ltype, errret) \ stac(); \ __asm__ __volatile__( \ - "1: mov"itype" %"rtype"1,%2\n" \ + GUARD( \ + " guest_access_mask_ptr %[ptr], %[scr1], %[scr2]\n" \ + ) \ + "1: mov"itype" %"rtype"[val], (%[ptr])\n" \ "2:\n" \ ".section .fixup,\"ax\"\n" \ - "3: mov %3,%0\n" \ + "3: mov %[errno], %[ret]\n" \ " jmp 2b\n" \ ".previous\n" \ _ASM_EXTABLE(1b, 3b) \ - : "=r"(err) \ - : ltype (x), "m"(__m(addr)), "i"(errret), "0"(err)); \ + : [ret] "+r" (err), [ptr] "=&r" (dummy_) \ + GUARD(, [scr1] "=&r" (dummy_), [scr2] "=&r" (dummy_)) \ + : [val] ltype (x), "m" (__m(addr)), \ + "[ptr]" (addr), [errno] "i" (errret)); \ clac() -#define get_unsafe_asm(x, addr, err, itype, rtype, ltype, errret) \ +#define get_unsafe_asm(x, addr, GUARD, err, rtype, ltype, errret) \ stac(); \ __asm__ __volatile__( \ - "1: mov"itype" %2,%"rtype"1\n" \ + GUARD( \ + " guest_access_mask_ptr %[ptr], %[scr1], %[scr2]\n" \ + ) \ + "1: mov (%[ptr]), %"rtype"[val]\n" \ "2:\n" \ ".section .fixup,\"ax\"\n" \ - "3: mov %3,%0\n" \ - " xor"itype" %"rtype"1,%"rtype"1\n" \ + "3: mov %[errno], %[ret]\n" \ + " xor %k[val], %k[val]\n" \ " jmp 2b\n" \ ".previous\n" \ _ASM_EXTABLE(1b, 3b) \ - : "=r"(err), ltype (x) \ - : "m"(__m(addr)), "i"(errret), "0"(err)); \ + : [ret] "+r" (err), [val] ltype (x), \ + [ptr] "=&r" (dummy_) \ + GUARD(, [scr1] "=&r" (dummy_), [scr2] "=&r" (dummy_)) \ + : "m" (__m(addr)), "[ptr]" (addr), \ + [errno] "i" (errret)); \ clac() -#define put_unsafe_size(x, ptr, size, retval, errret) \ +#define put_unsafe_size(x, ptr, size, grd, retval, errret) \ do { \ retval = 0; \ switch ( size ) \ { \ - case 1: put_unsafe_asm(x, ptr, retval, "b", "b", "iq", errret); break; \ - case 2: put_unsafe_asm(x, ptr, retval, "w", "w", "ir", errret); break; \ - case 4: put_unsafe_asm(x, ptr, retval, "l", "k", "ir", errret); break; \ - case 8: put_unsafe_asm(x, ptr, retval, "q", "", "ir", errret); break; \ + long dummy_; \ + case 1: \ + put_unsafe_asm(x, ptr, grd, retval, "b", "b", "iq", errret); \ + break; \ + case 2: \ + put_unsafe_asm(x, ptr, grd, retval, "w", "w", "ir", errret); \ + break; \ + case 4: \ + put_unsafe_asm(x, ptr, grd, retval, "l", "k", "ir", errret); \ + break; \ + case 8: \ + put_unsafe_asm(x, ptr, grd, retval, "q", "", "ir", errret); \ + break; \ default: __put_user_bad(); \ } \ } while ( false ) -#define put_guest_size put_unsafe_size -#define get_unsafe_size(x, ptr, size, retval, errret) \ +#define put_guest_size(x, ptr, size, retval, errret) \ + put_unsafe_size(x, ptr, size, UA_KEEP, retval, errret) + +#define get_unsafe_size(x, ptr, size, grd, retval, errret) \ do { \ retval = 0; \ switch ( size ) \ { \ - case 1: get_unsafe_asm(x, ptr, retval, "b", "b", "=q", errret); break; \ - case 2: get_unsafe_asm(x, ptr, retval, "w", "w", "=r", errret); break; \ - case 4: get_unsafe_asm(x, ptr, retval, "l", "k", "=r", errret); break; \ - case 8: get_unsafe_asm(x, ptr, retval, "q", "", "=r", errret); break; \ + long dummy_; \ + case 1: get_unsafe_asm(x, ptr, grd, retval, "b", "=q", errret); break; \ + case 2: get_unsafe_asm(x, ptr, grd, retval, "w", "=r", errret); break; \ + case 4: get_unsafe_asm(x, ptr, grd, retval, "k", "=r", errret); break; \ + case 8: get_unsafe_asm(x, ptr, grd, retval, "", "=r", errret); break; \ default: __get_user_bad(); \ } \ } while ( false ) -#define get_guest_size get_unsafe_size + +#define get_guest_size(x, ptr, size, retval, errret) \ + get_unsafe_size(x, ptr, size, UA_KEEP, retval, errret) /** * __copy_to_guest_pv: - Copy a block of data into guest space, with less @@ -229,9 +272,8 @@ __copy_to_guest_pv(void __user *to, cons return ret; } } - return __copy_to_user_ll(to, from, n); + return copy_to_guest_ll(to, from, n); } -#define copy_to_unsafe __copy_to_guest_pv /** * __copy_from_guest_pv: - Copy a block of data from guest space, with less @@ -270,9 +312,87 @@ __copy_from_guest_pv(void *to, const voi return ret; } } - return __copy_from_user_ll(to, from, n); + return copy_from_guest_ll(to, from, n); +} + +/** + * copy_to_unsafe: - Copy a block of data to unsafe space, with exception + * checking + * @to: Unsafe destination address. + * @from: Safe source address, in hypervisor space. + * @n: Number of bytes to copy. + * + * Copy data from hypervisor space to a potentially unmapped area. + * + * Returns number of bytes that could not be copied. + * On success, this will be zero. + */ +static always_inline unsigned int +copy_to_unsafe(void __user *to, const void *from, unsigned int n) +{ + if (__builtin_constant_p(n)) { + unsigned long ret; + + switch (n) { + case 1: + put_unsafe_size(*(const uint8_t *)from, to, 1, UA_DROP, ret, 1); + return ret; + case 2: + put_unsafe_size(*(const uint16_t *)from, to, 2, UA_DROP, ret, 2); + return ret; + case 4: + put_unsafe_size(*(const uint32_t *)from, to, 4, UA_DROP, ret, 4); + return ret; + case 8: + put_unsafe_size(*(const uint64_t *)from, to, 8, UA_DROP, ret, 8); + return ret; + } + } + + return copy_to_unsafe_ll(to, from, n); +} + +/** + * copy_from_unsafe: - Copy a block of data from unsafe space, with exception + * checking + * @to: Safe destination address, in hypervisor space. + * @from: Unsafe source address. + * @n: Number of bytes to copy. + * + * Copy data from a potentially unmapped area space to hypervisor space. + * + * Returns number of bytes that could not be copied. + * On success, this will be zero. + * + * If some data could not be copied, this function will pad the copied + * data to the requested size using zero bytes. + */ +static always_inline unsigned int +copy_from_unsafe(void *to, const void __user *from, unsigned int n) +{ + if ( __builtin_constant_p(n) ) + { + unsigned long ret; + + switch ( n ) + { + case 1: + get_unsafe_size(*(uint8_t *)to, from, 1, UA_DROP, ret, 1); + return ret; + case 2: + get_unsafe_size(*(uint16_t *)to, from, 2, UA_DROP, ret, 2); + return ret; + case 4: + get_unsafe_size(*(uint32_t *)to, from, 4, UA_DROP, ret, 4); + return ret; + case 8: + get_unsafe_size(*(uint64_t *)to, from, 8, UA_DROP, ret, 8); + return ret; + } + } + + return copy_from_unsafe_ll(to, from, n); } -#define copy_from_unsafe __copy_from_guest_pv /* * The exception table consists of pairs of addresses: the first is the
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |