* [PATCH v2 1/5] Kconfig: Add option for asm goto w/ tied outputs to workaround clang-13 bug
2022-02-02 0:49 [PATCH v2 0/5] x86: uaccess CMPXCHG + KVM bug fixes Sean Christopherson
@ 2022-02-02 0:49 ` Sean Christopherson
2022-02-02 0:49 ` [PATCH v2 2/5] x86/uaccess: Implement macros for CMPXCHG on user addresses Sean Christopherson
` (4 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Sean Christopherson @ 2022-02-02 0:49 UTC (permalink / raw)
To: Paolo Bonzini, Nathan Chancellor, Nick Desaulniers
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, llvm, linux-kernel, Peter Zijlstra,
Tadeusz Struk, syzbot+6cde2282daa792c49ab8
Add a config option to guard (future) usage of asm_volatile_goto() that
includes "tied outputs", i.e. "+" constraints that specify both an input
and output parameter. clang-13 has a bug[1] that causes compilation of
such inline asm to fail, and KVM wants to use a "+m" constraint to
implement a uaccess form of CMPXCHG[2]. E.g. the test code fails with
<stdin>:1:29: error: invalid operand in inline asm: '.long (${1:l}) - .'
int foo(int *x) { asm goto (".long (%l[bar]) - .\n": "+m"(*x) ::: bar); return *x; bar: return 0; }
^
<stdin>:1:29: error: unknown token in expression
<inline asm>:1:9: note: instantiated into assembly here
.long () - .
^
2 errors generated.
on clang-13, but passes on gcc (with appropriate asm goto support). The
bug is fixed in clang-14, but won't be backported to clang-13 as the
changes are too invasive/risky.
gcc also had a similar bug[3], fixed in gcc-11, where gcc failed to
account for its behavior of assigning two numbers to tied outputs (one
for input, one for output) when evaluating symbolic references.
[1] https://github.com/ClangBuiltLinux/linux/issues/1512
[2] https://lore.kernel.org/all/YfMruK8%2F1izZ2VHS@google.com
[3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98096
Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
init/Kconfig | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/init/Kconfig b/init/Kconfig
index e9119bf54b1f..4a7a569706c5 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -77,6 +77,11 @@ config CC_HAS_ASM_GOTO_OUTPUT
depends on CC_HAS_ASM_GOTO
def_bool $(success,echo 'int foo(int x) { asm goto ("": "=r"(x) ::: bar); return x; bar: return 0; }' | $(CC) -x c - -c -o /dev/null)
+config CC_HAS_ASM_GOTO_TIED_OUTPUT
+ depends on CC_HAS_ASM_GOTO_OUTPUT
+ # Detect buggy gcc and clang, fixed in gcc-11 clang-14.
+ def_bool $(success,echo 'int foo(int *x) { asm goto (".long (%l[bar]) - .\n": "+m"(*x) ::: bar); return *x; bar: return 0; }' | $CC -x c - -c -o /dev/null)
+
config TOOLS_SUPPORT_RELR
def_bool $(success,env "CC=$(CC)" "LD=$(LD)" "NM=$(NM)" "OBJCOPY=$(OBJCOPY)" $(srctree)/scripts/tools-support-relr.sh)
--
2.35.0.rc2.247.g8bbb082509-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 2/5] x86/uaccess: Implement macros for CMPXCHG on user addresses
2022-02-02 0:49 [PATCH v2 0/5] x86: uaccess CMPXCHG + KVM bug fixes Sean Christopherson
2022-02-02 0:49 ` [PATCH v2 1/5] Kconfig: Add option for asm goto w/ tied outputs to workaround clang-13 bug Sean Christopherson
@ 2022-02-02 0:49 ` Sean Christopherson
2022-02-02 0:49 ` [PATCH v2 3/5] KVM: x86: Use __try_cmpxchg_user() to update guest PTE A/D bits Sean Christopherson
` (3 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Sean Christopherson @ 2022-02-02 0:49 UTC (permalink / raw)
To: Paolo Bonzini, Nathan Chancellor, Nick Desaulniers
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, llvm, linux-kernel, Peter Zijlstra,
Tadeusz Struk, syzbot+6cde2282daa792c49ab8
From: Peter Zijlstra <peterz@infradead.org>
Add support for CMPXCHG loops on userspace addresses. Provide both an
"unsafe" version for tight loops that do their own uaccess begin/end, as
well as a "safe" version for use cases where the CMPXCHG is not buried in
a loop, e.g. KVM will resume the guest instead of looping when emulation
of a guest atomic accesses fails the CMPXCHG.
Provide 8-byte versions for 32-bit kernels so that KVM can do CMPXCHG on
guest PAE PTEs, which are accessed via userspace addresses.
Guard the asm_volatile_goto() variation with CC_HAS_ASM_GOTO_TIED_OUTPUT,
the "+m" constraint fails on some compilers that otherwise support
CC_HAS_ASM_GOTO_OUTPUT.
Cc: stable@vger.kernel.org
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Co-developed-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/include/asm/uaccess.h | 142 +++++++++++++++++++++++++++++++++
1 file changed, 142 insertions(+)
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index ac96f9b2d64b..1c14bcce88f2 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -409,6 +409,103 @@ do { \
#endif // CONFIG_CC_HAS_ASM_GOTO_OUTPUT
+#ifdef CONFIG_CC_HAS_ASM_GOTO_TIED_OUTPUT
+#define __try_cmpxchg_user_asm(itype, ltype, _ptr, _pold, _new, label) ({ \
+ bool success; \
+ __typeof__(_ptr) _old = (__typeof__(_ptr))(_pold); \
+ __typeof__(*(_ptr)) __old = *_old; \
+ __typeof__(*(_ptr)) __new = (_new); \
+ asm_volatile_goto("\n" \
+ "1: " LOCK_PREFIX "cmpxchg"itype" %[new], %[ptr]\n"\
+ _ASM_EXTABLE_UA(1b, %l[label]) \
+ : CC_OUT(z) (success), \
+ [ptr] "+m" (*_ptr), \
+ [old] "+a" (__old) \
+ : [new] ltype (__new) \
+ : "memory" \
+ : label); \
+ if (unlikely(!success)) \
+ *_old = __old; \
+ likely(success); })
+
+#ifdef CONFIG_X86_32
+#define __try_cmpxchg64_user_asm(_ptr, _pold, _new, label) ({ \
+ bool success; \
+ __typeof__(_ptr) _old = (__typeof__(_ptr))(_pold); \
+ __typeof__(*(_ptr)) __old = *_old; \
+ __typeof__(*(_ptr)) __new = (_new); \
+ asm_volatile_goto("\n" \
+ "1: " LOCK_PREFIX "cmpxchg8b %[ptr]\n" \
+ _ASM_EXTABLE_UA(1b, %l[label]) \
+ : CC_OUT(z) (success), \
+ "+A" (__old), \
+ [ptr] "+m" (*_ptr) \
+ : "b" ((u32)__new), \
+ "c" ((u32)((u64)__new >> 32)) \
+ : "memory" \
+ : label); \
+ if (unlikely(!success)) \
+ *_old = __old; \
+ likely(success); })
+#endif // CONFIG_X86_32
+#else // !CONFIG_CC_HAS_ASM_GOTO_TIED_OUTPUT
+#define __try_cmpxchg_user_asm(itype, ltype, _ptr, _pold, _new, label) ({ \
+ int __err = 0; \
+ bool success; \
+ __typeof__(_ptr) _old = (__typeof__(_ptr))(_pold); \
+ __typeof__(*(_ptr)) __old = *_old; \
+ __typeof__(*(_ptr)) __new = (_new); \
+ asm volatile("\n" \
+ "1: " LOCK_PREFIX "cmpxchg"itype" %[new], %[ptr]\n"\
+ CC_SET(z) \
+ "2:\n" \
+ _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG, \
+ %[errout]) \
+ : CC_OUT(z) (success), \
+ [errout] "+r" (__err), \
+ [ptr] "+m" (*_ptr), \
+ [old] "+a" (__old) \
+ : [new] ltype (__new) \
+ : "memory", "cc"); \
+ if (unlikely(__err)) \
+ goto label; \
+ if (unlikely(!success)) \
+ *_old = __old; \
+ likely(success); })
+
+#ifdef CONFIG_X86_32
+/*
+ * Unlike the normal CMPXCHG, hardcode ECX for both success/fail and error.
+ * There are only six GPRs available and four (EAX, EBX, ECX, and EDX) are
+ * hardcoded by CMPXCHG8B, leaving only ESI and EDI. If the compiler uses
+ * both ESI and EDI for the memory operand, compilation will fail if the error
+ * is an input+output as there will be no register available for input.
+ */
+#define __try_cmpxchg64_user_asm(_ptr, _pold, _new, label) ({ \
+ int __result; \
+ __typeof__(_ptr) _old = (__typeof__(_ptr))(_pold); \
+ __typeof__(*(_ptr)) __old = *_old; \
+ __typeof__(*(_ptr)) __new = (_new); \
+ asm volatile("\n" \
+ "1: " LOCK_PREFIX "cmpxchg8b %[ptr]\n" \
+ "mov $0, %%ecx\n\t" \
+ "setz %%cl\n" \
+ "2:\n" \
+ _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG, %%ecx) \
+ : [result]"=c" (__result), \
+ "+A" (__old), \
+ [ptr] "+m" (*_ptr) \
+ : "b" ((u32)__new), \
+ "c" ((u32)((u64)__new >> 32)) \
+ : "memory", "cc"); \
+ if (unlikely(__result < 0)) \
+ goto label; \
+ if (unlikely(!__result)) \
+ *_old = __old; \
+ likely(__result); })
+#endif // CONFIG_X86_32
+#endif // CONFIG_CC_HAS_ASM_GOTO_TIED_OUTPUT
+
/* FIXME: this hack is definitely wrong -AK */
struct __large_struct { unsigned long buf[100]; };
#define __m(x) (*(struct __large_struct __user *)(x))
@@ -501,6 +598,51 @@ do { \
} while (0)
#endif // CONFIG_CC_HAS_ASM_GOTO_OUTPUT
+extern void __try_cmpxchg_user_wrong_size(void);
+
+#ifndef CONFIG_X86_32
+#define __try_cmpxchg64_user_asm(_ptr, _oldp, _nval, _label) \
+ __try_cmpxchg_user_asm("q", "r", (_ptr), (_oldp), (_nval), _label)
+#endif
+
+/*
+ * Force the pointer to u<size> to match the size expected by the asm helper.
+ * clang/LLVM compiles all cases and only discards the unused paths after
+ * processing errors, which breaks i386 if the pointer is an 8-byte value.
+ */
+#define unsafe_try_cmpxchg_user(_ptr, _oldp, _nval, _label) ({ \
+ bool __ret; \
+ __chk_user_ptr(_ptr); \
+ switch (sizeof(*(_ptr))) { \
+ case 1: __ret = __try_cmpxchg_user_asm("b", "q", \
+ (__force u8 *)(_ptr), (_oldp), \
+ (_nval), _label); \
+ break; \
+ case 2: __ret = __try_cmpxchg_user_asm("w", "r", \
+ (__force u16 *)(_ptr), (_oldp), \
+ (_nval), _label); \
+ break; \
+ case 4: __ret = __try_cmpxchg_user_asm("l", "r", \
+ (__force u32 *)(_ptr), (_oldp), \
+ (_nval), _label); \
+ break; \
+ case 8: __ret = __try_cmpxchg64_user_asm((__force u64 *)(_ptr), (_oldp),\
+ (_nval), _label); \
+ break; \
+ default: __try_cmpxchg_user_wrong_size(); \
+ } \
+ __ret; })
+
+/* "Returns" 0 on success, 1 on failure, -EFAULT if the access faults. */
+#define __try_cmpxchg_user(_ptr, _oldp, _nval, _label) ({ \
+ int __ret = -EFAULT; \
+ __uaccess_begin_nospec(); \
+ __ret = !unsafe_try_cmpxchg_user(_ptr, _oldp, _nval, _label); \
+_label: \
+ __uaccess_end(); \
+ __ret; \
+ })
+
/*
* We want the unsafe accessors to always be inlined and use
* the error labels - thus the macro games.
--
2.35.0.rc2.247.g8bbb082509-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 3/5] KVM: x86: Use __try_cmpxchg_user() to update guest PTE A/D bits
2022-02-02 0:49 [PATCH v2 0/5] x86: uaccess CMPXCHG + KVM bug fixes Sean Christopherson
2022-02-02 0:49 ` [PATCH v2 1/5] Kconfig: Add option for asm goto w/ tied outputs to workaround clang-13 bug Sean Christopherson
2022-02-02 0:49 ` [PATCH v2 2/5] x86/uaccess: Implement macros for CMPXCHG on user addresses Sean Christopherson
@ 2022-02-02 0:49 ` Sean Christopherson
2022-02-02 0:49 ` [PATCH v2 4/5] KVM: x86: Use __try_cmpxchg_user() to emulate atomic accesses Sean Christopherson
` (2 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Sean Christopherson @ 2022-02-02 0:49 UTC (permalink / raw)
To: Paolo Bonzini, Nathan Chancellor, Nick Desaulniers
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, llvm, linux-kernel, Peter Zijlstra,
Tadeusz Struk, syzbot+6cde2282daa792c49ab8
Use the recently introduced __try_cmpxchg_user() to update guest PTE A/D
bits instead of mapping the PTE into kernel address space. The VM_PFNMAP
path is broken as it assumes that vm_pgoff is the base pfn of the mapped
VMA range, which is conceptually wrong as vm_pgoff is the offset relative
to the file and has nothing to do with the pfn. The horrific hack worked
for the original use case (backing guest memory with /dev/mem), but leads
to accessing "random" pfns for pretty much any other VM_PFNMAP case.
Fixes: bd53cb35a3e9 ("X86/KVM: Handle PFNs outside of kernel reach when touching GPTEs")
Debugged-by: Tadeusz Struk <tadeusz.struk@linaro.org>
Tested-by: Tadeusz Struk <tadeusz.struk@linaro.org>
Reported-by: syzbot+6cde2282daa792c49ab8@syzkaller.appspotmail.com
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/mmu/paging_tmpl.h | 45 +---------------------------------
1 file changed, 1 insertion(+), 44 deletions(-)
diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
index 5b5bdac97c7b..551de15f342f 100644
--- a/arch/x86/kvm/mmu/paging_tmpl.h
+++ b/arch/x86/kvm/mmu/paging_tmpl.h
@@ -143,49 +143,6 @@ static bool FNAME(is_rsvd_bits_set)(struct kvm_mmu *mmu, u64 gpte, int level)
FNAME(is_bad_mt_xwr)(&mmu->guest_rsvd_check, gpte);
}
-static int FNAME(cmpxchg_gpte)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
- pt_element_t __user *ptep_user, unsigned index,
- pt_element_t orig_pte, pt_element_t new_pte)
-{
- int npages;
- pt_element_t ret;
- pt_element_t *table;
- struct page *page;
-
- npages = get_user_pages_fast((unsigned long)ptep_user, 1, FOLL_WRITE, &page);
- if (likely(npages == 1)) {
- table = kmap_atomic(page);
- ret = CMPXCHG(&table[index], orig_pte, new_pte);
- kunmap_atomic(table);
-
- kvm_release_page_dirty(page);
- } else {
- struct vm_area_struct *vma;
- unsigned long vaddr = (unsigned long)ptep_user & PAGE_MASK;
- unsigned long pfn;
- unsigned long paddr;
-
- mmap_read_lock(current->mm);
- vma = find_vma_intersection(current->mm, vaddr, vaddr + PAGE_SIZE);
- if (!vma || !(vma->vm_flags & VM_PFNMAP)) {
- mmap_read_unlock(current->mm);
- return -EFAULT;
- }
- pfn = ((vaddr - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
- paddr = pfn << PAGE_SHIFT;
- table = memremap(paddr, PAGE_SIZE, MEMREMAP_WB);
- if (!table) {
- mmap_read_unlock(current->mm);
- return -EFAULT;
- }
- ret = CMPXCHG(&table[index], orig_pte, new_pte);
- memunmap(table);
- mmap_read_unlock(current->mm);
- }
-
- return (ret != orig_pte);
-}
-
static bool FNAME(prefetch_invalid_gpte)(struct kvm_vcpu *vcpu,
struct kvm_mmu_page *sp, u64 *spte,
u64 gpte)
@@ -284,7 +241,7 @@ static int FNAME(update_accessed_dirty_bits)(struct kvm_vcpu *vcpu,
if (unlikely(!walker->pte_writable[level - 1]))
continue;
- ret = FNAME(cmpxchg_gpte)(vcpu, mmu, ptep_user, index, orig_pte, pte);
+ ret = __try_cmpxchg_user(ptep_user, &orig_pte, pte, fault);
if (ret)
return ret;
--
2.35.0.rc2.247.g8bbb082509-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 4/5] KVM: x86: Use __try_cmpxchg_user() to emulate atomic accesses
2022-02-02 0:49 [PATCH v2 0/5] x86: uaccess CMPXCHG + KVM bug fixes Sean Christopherson
` (2 preceding siblings ...)
2022-02-02 0:49 ` [PATCH v2 3/5] KVM: x86: Use __try_cmpxchg_user() to update guest PTE A/D bits Sean Christopherson
@ 2022-02-02 0:49 ` Sean Christopherson
2022-05-12 10:14 ` [PATCH] KVM: x86: fix a typo in __try_cmpxchg_user that caused cmpxchg to be not atomic Maxim Levitsky
2022-02-02 0:49 ` [PATCH v2 5/5] KVM: x86: Bail to userspace if emulation of atomic user access faults Sean Christopherson
2022-04-01 7:53 ` [PATCH v2 0/5] x86: uaccess CMPXCHG + KVM bug fixes Paolo Bonzini
5 siblings, 1 reply; 15+ messages in thread
From: Sean Christopherson @ 2022-02-02 0:49 UTC (permalink / raw)
To: Paolo Bonzini, Nathan Chancellor, Nick Desaulniers
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, llvm, linux-kernel, Peter Zijlstra,
Tadeusz Struk, syzbot+6cde2282daa792c49ab8
Use the recently introduce __try_cmpxchg_user() to emulate atomic guest
accesses via the associated userspace address instead of mapping the
backing pfn into kernel address space. Using kvm_vcpu_map() is unsafe as
it does not coordinate with KVM's mmu_notifier to ensure the hva=>pfn
translation isn't changed/unmapped in the memremap() path, i.e. when
there's no struct page and thus no elevated refcount.
Fixes: 42e35f8072c3 ("KVM/X86: Use kvm_vcpu_map in emulator_cmpxchg_emulated")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/x86.c | 35 ++++++++++++++---------------------
1 file changed, 14 insertions(+), 21 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 74b53a16f38a..c9cac3100f77 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7155,15 +7155,8 @@ static int emulator_write_emulated(struct x86_emulate_ctxt *ctxt,
exception, &write_emultor);
}
-#define CMPXCHG_TYPE(t, ptr, old, new) \
- (cmpxchg((t *)(ptr), *(t *)(old), *(t *)(new)) == *(t *)(old))
-
-#ifdef CONFIG_X86_64
-# define CMPXCHG64(ptr, old, new) CMPXCHG_TYPE(u64, ptr, old, new)
-#else
-# define CMPXCHG64(ptr, old, new) \
- (cmpxchg64((u64 *)(ptr), *(u64 *)(old), *(u64 *)(new)) == *(u64 *)(old))
-#endif
+#define emulator_try_cmpxchg_user(t, ptr, old, new) \
+ (__try_cmpxchg_user((t __user *)(ptr), (t *)(old), *(t *)(new), efault ## t))
static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
unsigned long addr,
@@ -7172,12 +7165,11 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
unsigned int bytes,
struct x86_exception *exception)
{
- struct kvm_host_map map;
struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
u64 page_line_mask;
+ unsigned long hva;
gpa_t gpa;
- char *kaddr;
- bool exchanged;
+ int r;
/* guests cmpxchg8b have to be emulated atomically */
if (bytes > 8 || (bytes & (bytes - 1)))
@@ -7201,31 +7193,32 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
if (((gpa + bytes - 1) & page_line_mask) != (gpa & page_line_mask))
goto emul_write;
- if (kvm_vcpu_map(vcpu, gpa_to_gfn(gpa), &map))
+ hva = kvm_vcpu_gfn_to_hva(vcpu, gpa_to_gfn(gpa));
+ if (kvm_is_error_hva(addr))
goto emul_write;
- kaddr = map.hva + offset_in_page(gpa);
+ hva += offset_in_page(gpa);
switch (bytes) {
case 1:
- exchanged = CMPXCHG_TYPE(u8, kaddr, old, new);
+ r = emulator_try_cmpxchg_user(u8, hva, old, new);
break;
case 2:
- exchanged = CMPXCHG_TYPE(u16, kaddr, old, new);
+ r = emulator_try_cmpxchg_user(u16, hva, old, new);
break;
case 4:
- exchanged = CMPXCHG_TYPE(u32, kaddr, old, new);
+ r = emulator_try_cmpxchg_user(u32, hva, old, new);
break;
case 8:
- exchanged = CMPXCHG64(kaddr, old, new);
+ r = emulator_try_cmpxchg_user(u64, hva, old, new);
break;
default:
BUG();
}
- kvm_vcpu_unmap(vcpu, &map, true);
-
- if (!exchanged)
+ if (r < 0)
+ goto emul_write;
+ if (r)
return X86EMUL_CMPXCHG_FAILED;
kvm_page_track_write(vcpu, gpa, new, bytes);
--
2.35.0.rc2.247.g8bbb082509-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH] KVM: x86: fix a typo in __try_cmpxchg_user that caused cmpxchg to be not atomic
2022-02-02 0:49 ` [PATCH v2 4/5] KVM: x86: Use __try_cmpxchg_user() to emulate atomic accesses Sean Christopherson
@ 2022-05-12 10:14 ` Maxim Levitsky
2022-05-12 12:45 ` Vitaly Kuznetsov
2022-05-12 15:47 ` Paolo Bonzini
0 siblings, 2 replies; 15+ messages in thread
From: Maxim Levitsky @ 2022-05-12 10:14 UTC (permalink / raw)
To: kvm
Cc: Wanpeng Li, Dave Hansen, Joerg Roedel, Sean Christopherson,
linux-kernel, H. Peter Anvin, Paolo Bonzini, Jim Mattson, x86,
Ingo Molnar, Borislav Petkov, Vitaly Kuznetsov, Thomas Gleixner,
Maxim Levitsky
Fixes: 1c2361f667f36 ("KVM: x86: Use __try_cmpxchg_user() to emulate atomic accesses")
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
arch/x86/kvm/x86.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Yes, this is the root cause of the TDP mmu leak I was doing debug of in the last week.
Non working cmpxchg on which TDP mmu relies makes it install two differnt shadow pages
under same spte.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8ee8c91fa7625..79cabd3d97d22 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7329,7 +7329,7 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
goto emul_write;
hva = kvm_vcpu_gfn_to_hva(vcpu, gpa_to_gfn(gpa));
- if (kvm_is_error_hva(addr))
+ if (kvm_is_error_hva(hva))
goto emul_write;
hva += offset_in_page(gpa);
--
2.26.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] KVM: x86: fix a typo in __try_cmpxchg_user that caused cmpxchg to be not atomic
2022-05-12 10:14 ` [PATCH] KVM: x86: fix a typo in __try_cmpxchg_user that caused cmpxchg to be not atomic Maxim Levitsky
@ 2022-05-12 12:45 ` Vitaly Kuznetsov
2022-05-12 13:48 ` Sean Christopherson
2022-05-12 15:47 ` Paolo Bonzini
1 sibling, 1 reply; 15+ messages in thread
From: Vitaly Kuznetsov @ 2022-05-12 12:45 UTC (permalink / raw)
To: Maxim Levitsky, kvm
Cc: Wanpeng Li, Dave Hansen, Joerg Roedel, Sean Christopherson,
linux-kernel, H. Peter Anvin, Paolo Bonzini, Jim Mattson, x86,
Ingo Molnar, Borislav Petkov, Thomas Gleixner, Maxim Levitsky
Maxim Levitsky <mlevitsk@redhat.com> writes:
> Fixes: 1c2361f667f36 ("KVM: x86: Use __try_cmpxchg_user() to emulate atomic accesses")
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
> arch/x86/kvm/x86.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Yes, this is the root cause of the TDP mmu leak I was doing debug of in the last week.
> Non working cmpxchg on which TDP mmu relies makes it install two differnt shadow pages
> under same spte.
In case the fix is not squashed with 1c2361f667f36, the above should
really go before '---'.
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 8ee8c91fa7625..79cabd3d97d22 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7329,7 +7329,7 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
> goto emul_write;
>
> hva = kvm_vcpu_gfn_to_hva(vcpu, gpa_to_gfn(gpa));
> - if (kvm_is_error_hva(addr))
> + if (kvm_is_error_hva(hva))
Looks like a typo indeed, so
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> goto emul_write;
>
> hva += offset_in_page(gpa);
--
Vitaly
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] KVM: x86: fix a typo in __try_cmpxchg_user that caused cmpxchg to be not atomic
2022-05-12 12:45 ` Vitaly Kuznetsov
@ 2022-05-12 13:48 ` Sean Christopherson
0 siblings, 0 replies; 15+ messages in thread
From: Sean Christopherson @ 2022-05-12 13:48 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Maxim Levitsky, kvm, Wanpeng Li, Dave Hansen, Joerg Roedel,
linux-kernel, H. Peter Anvin, Paolo Bonzini, Jim Mattson, x86,
Ingo Molnar, Borislav Petkov, Thomas Gleixner
On Thu, May 12, 2022, Vitaly Kuznetsov wrote:
> Maxim Levitsky <mlevitsk@redhat.com> writes:
>
> > Fixes: 1c2361f667f36 ("KVM: x86: Use __try_cmpxchg_user() to emulate atomic accesses")
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> > arch/x86/kvm/x86.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > Yes, this is the root cause of the TDP mmu leak I was doing debug of in the last week.
> > Non working cmpxchg on which TDP mmu relies makes it install two differnt shadow pages
> > under same spte.
Ewww, as in running a buggy L0 resulted in a CMPXCHG going sideways in L1? That's
awful. My apologies :-(
> In case the fix is not squashed with 1c2361f667f36, the above should
> really go before '---'.
>
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 8ee8c91fa7625..79cabd3d97d22 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -7329,7 +7329,7 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
> > goto emul_write;
> >
> > hva = kvm_vcpu_gfn_to_hva(vcpu, gpa_to_gfn(gpa));
> > - if (kvm_is_error_hva(addr))
> > + if (kvm_is_error_hva(hva))
>
> Looks like a typo indeed, so
>
> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Yep, if this doesn't get squashed
Reviewed-by: Sean Christopherson <seanjc@google.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] KVM: x86: fix a typo in __try_cmpxchg_user that caused cmpxchg to be not atomic
2022-05-12 10:14 ` [PATCH] KVM: x86: fix a typo in __try_cmpxchg_user that caused cmpxchg to be not atomic Maxim Levitsky
2022-05-12 12:45 ` Vitaly Kuznetsov
@ 2022-05-12 15:47 ` Paolo Bonzini
2022-05-12 21:27 ` Sean Christopherson
1 sibling, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2022-05-12 15:47 UTC (permalink / raw)
To: Maxim Levitsky, kvm
Cc: Wanpeng Li, Dave Hansen, Joerg Roedel, Sean Christopherson,
linux-kernel, H. Peter Anvin, Jim Mattson, x86, Ingo Molnar,
Borislav Petkov, Vitaly Kuznetsov, Thomas Gleixner
On 5/12/22 12:14, Maxim Levitsky wrote:
> Yes, this is the root cause of the TDP mmu leak I was doing debug of in the last week.
> Non working cmpxchg on which TDP mmu relies makes it install two differnt shadow pages
> under same spte.
Awesome! And queued, thanks.
Paolo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] KVM: x86: fix a typo in __try_cmpxchg_user that caused cmpxchg to be not atomic
2022-05-12 15:47 ` Paolo Bonzini
@ 2022-05-12 21:27 ` Sean Christopherson
2022-05-16 13:14 ` Maxim Levitsky
0 siblings, 1 reply; 15+ messages in thread
From: Sean Christopherson @ 2022-05-12 21:27 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Maxim Levitsky, kvm, Wanpeng Li, Dave Hansen, Joerg Roedel,
linux-kernel, H. Peter Anvin, Jim Mattson, x86, Ingo Molnar,
Borislav Petkov, Vitaly Kuznetsov, Thomas Gleixner
On Thu, May 12, 2022, Paolo Bonzini wrote:
> On 5/12/22 12:14, Maxim Levitsky wrote:
> > Yes, this is the root cause of the TDP mmu leak I was doing debug of in the last week.
> > Non working cmpxchg on which TDP mmu relies makes it install two differnt shadow pages
> > under same spte.
>
> Awesome! And queued, thanks.
If you haven't done so already, can you add
Cc: stable@vger.kernel.org
Also, given that we have concrete proof that not honoring atomic accesses can have
dire consequences for the guest, what about adding a capability to turn the emul_write
path into an emulation error?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] KVM: x86: fix a typo in __try_cmpxchg_user that caused cmpxchg to be not atomic
2022-05-12 21:27 ` Sean Christopherson
@ 2022-05-16 13:14 ` Maxim Levitsky
2022-05-16 14:03 ` Sean Christopherson
0 siblings, 1 reply; 15+ messages in thread
From: Maxim Levitsky @ 2022-05-16 13:14 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini
Cc: kvm, Wanpeng Li, Dave Hansen, Joerg Roedel, linux-kernel,
H. Peter Anvin, Jim Mattson, x86, Ingo Molnar, Borislav Petkov,
Vitaly Kuznetsov, Thomas Gleixner
On Thu, 2022-05-12 at 21:27 +0000, Sean Christopherson wrote:
> On Thu, May 12, 2022, Paolo Bonzini wrote:
> > On 5/12/22 12:14, Maxim Levitsky wrote:
> > > Yes, this is the root cause of the TDP mmu leak I was doing debug of in the last week.
> > > Non working cmpxchg on which TDP mmu relies makes it install two differnt shadow pages
> > > under same spte.
> >
> > Awesome! And queued, thanks.
>
> If you haven't done so already, can you add
>
> Cc: stable@vger.kernel.org
When I posted my patch, I checked that the patch didn't reach mainline yet,
so I assumed that it won't be in -stable either yet, although it was CCed there.
>
> Also, given that we have concrete proof that not honoring atomic accesses can have
> dire consequences for the guest, what about adding a capability to turn the emul_write
> path into an emulation error?
>
This is a good idea. It might though break some guests - I did see that
warning few times, that is why I wasn't alert by the fact that it started showing up more often.
I'll take a look at this soon.
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] KVM: x86: fix a typo in __try_cmpxchg_user that caused cmpxchg to be not atomic
2022-05-16 13:14 ` Maxim Levitsky
@ 2022-05-16 14:03 ` Sean Christopherson
0 siblings, 0 replies; 15+ messages in thread
From: Sean Christopherson @ 2022-05-16 14:03 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Paolo Bonzini, kvm, Wanpeng Li, Dave Hansen, Joerg Roedel,
linux-kernel, H. Peter Anvin, Jim Mattson, x86, Ingo Molnar,
Borislav Petkov, Vitaly Kuznetsov, Thomas Gleixner
On Mon, May 16, 2022, Maxim Levitsky wrote:
> On Thu, 2022-05-12 at 21:27 +0000, Sean Christopherson wrote:
> > On Thu, May 12, 2022, Paolo Bonzini wrote:
> > > On 5/12/22 12:14, Maxim Levitsky wrote:
> > > > Yes, this is the root cause of the TDP mmu leak I was doing debug of in the last week.
> > > > Non working cmpxchg on which TDP mmu relies makes it install two differnt shadow pages
> > > > under same spte.
> > >
> > > Awesome! And queued, thanks.
> >
> > If you haven't done so already, can you add
> >
> > Cc: stable@vger.kernel.org
>
> When I posted my patch, I checked that the patch didn't reach mainline yet,
> so I assumed that it won't be in -stable either yet, although it was CCed there.
Yeah, it should hit stable trees because of the explicit stable@. The Fixes: on
this patch is likely enough, but no harm in being paranoid.
> > Also, given that we have concrete proof that not honoring atomic accesses can have
> > dire consequences for the guest, what about adding a capability to turn the emul_write
> > path into an emulation error?
> >
>
>
> This is a good idea. It might though break some guests - I did see that
> warning few times, that is why I wasn't alert by the fact that it started
> showing up more often.
It mostly shows up in KUT, one of the tests deliberately triggers the scenario.
But yeah, there's definitely potential for breakage. Not sure if a capability or
debug oriented module param would be best. In theory, userspace could do a better
job of emulating the atomic access than KVM, which makes me lean toward a capability,
but practically speaking I doubt a userspace will ever do anything besides
terminate the guest.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 5/5] KVM: x86: Bail to userspace if emulation of atomic user access faults
2022-02-02 0:49 [PATCH v2 0/5] x86: uaccess CMPXCHG + KVM bug fixes Sean Christopherson
` (3 preceding siblings ...)
2022-02-02 0:49 ` [PATCH v2 4/5] KVM: x86: Use __try_cmpxchg_user() to emulate atomic accesses Sean Christopherson
@ 2022-02-02 0:49 ` Sean Christopherson
2022-04-01 7:53 ` [PATCH v2 0/5] x86: uaccess CMPXCHG + KVM bug fixes Paolo Bonzini
5 siblings, 0 replies; 15+ messages in thread
From: Sean Christopherson @ 2022-02-02 0:49 UTC (permalink / raw)
To: Paolo Bonzini, Nathan Chancellor, Nick Desaulniers
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, llvm, linux-kernel, Peter Zijlstra,
Tadeusz Struk, syzbot+6cde2282daa792c49ab8
Exit to userspace when emulating an atomic guest access if the CMPXCHG on
the userspace address faults. Emulating the access as a write and thus
likely treating it as emulated MMIO is wrong, as KVM has already
confirmed there is a valid, writable memslot.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/x86.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c9cac3100f77..24e0981816c0 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7217,7 +7217,7 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
}
if (r < 0)
- goto emul_write;
+ return X86EMUL_UNHANDLEABLE;
if (r)
return X86EMUL_CMPXCHG_FAILED;
--
2.35.0.rc2.247.g8bbb082509-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 0/5] x86: uaccess CMPXCHG + KVM bug fixes
2022-02-02 0:49 [PATCH v2 0/5] x86: uaccess CMPXCHG + KVM bug fixes Sean Christopherson
` (4 preceding siblings ...)
2022-02-02 0:49 ` [PATCH v2 5/5] KVM: x86: Bail to userspace if emulation of atomic user access faults Sean Christopherson
@ 2022-04-01 7:53 ` Paolo Bonzini
2022-04-01 17:07 ` Tadeusz Struk
5 siblings, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2022-04-01 7:53 UTC (permalink / raw)
To: Sean Christopherson
Cc: Nathan Chancellor, Nick Desaulniers, Vitaly Kuznetsov,
Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, llvm, linux-kernel,
Peter Zijlstra, Tadeusz Struk, syzbot+6cde2282daa792c49ab8
Queued, thanks.
Paolo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 0/5] x86: uaccess CMPXCHG + KVM bug fixes
2022-04-01 7:53 ` [PATCH v2 0/5] x86: uaccess CMPXCHG + KVM bug fixes Paolo Bonzini
@ 2022-04-01 17:07 ` Tadeusz Struk
0 siblings, 0 replies; 15+ messages in thread
From: Tadeusz Struk @ 2022-04-01 17:07 UTC (permalink / raw)
To: Paolo Bonzini, Sean Christopherson
Cc: Nathan Chancellor, Nick Desaulniers, Vitaly Kuznetsov,
Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, llvm, linux-kernel,
Peter Zijlstra, syzbot+6cde2282daa792c49ab8
On 4/1/22 00:53, Paolo Bonzini wrote:
> Queued, thanks.
>
Hi Paolo,
Where is that queued on? https://git.kernel.org/pub/scm/virt/kvm/kvm.git/ ?
I don't see it there.
--
Thanks,
Tadeusz
^ permalink raw reply [flat|nested] 15+ messages in thread