* [PATCH 1/5] Kconfig: Add option for asm goto w/ tied outputs to workaround clang-13 bug
2022-02-01 1:08 [PATCH 0/5] x86: uaccess CMPXCHG + KVM bug fixes Sean Christopherson
@ 2022-02-01 1:08 ` Sean Christopherson
2022-02-01 20:16 ` Nick Desaulniers
2022-02-01 1:08 ` [PATCH 2/5] x86/uaccess: Implement macros for CMPXCHG on user addresses Sean Christopherson
` (4 subsequent siblings)
5 siblings, 1 reply; 10+ messages in thread
From: Sean Christopherson @ 2022-02-01 1:08 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,
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.
[1] https://github.com/ClangBuiltLinux/linux/issues/1512
[2] https://lore.kernel.org/all/YfMruK8%2F1izZ2VHS@google.com
Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
init/Kconfig | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/init/Kconfig b/init/Kconfig
index e9119bf54b1f..a206b21703be 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -77,6 +77,10 @@ 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
+ 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] 10+ messages in thread
* Re: [PATCH 1/5] Kconfig: Add option for asm goto w/ tied outputs to workaround clang-13 bug
2022-02-01 1:08 ` [PATCH 1/5] Kconfig: Add option for asm goto w/ tied outputs to workaround clang-13 bug Sean Christopherson
@ 2022-02-01 20:16 ` Nick Desaulniers
2022-02-01 20:56 ` Sean Christopherson
0 siblings, 1 reply; 10+ messages in thread
From: Nick Desaulniers @ 2022-02-01 20:16 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Nathan Chancellor, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel, kvm, llvm, linux-kernel,
Peter Zijlstra, syzbot+6cde2282daa792c49ab8
On Mon, Jan 31, 2022 at 5:08 PM Sean Christopherson <seanjc@google.com> wrote:
>
> 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.
LGTM.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
If you're going to respin the series, consider adding a comment in the
source along the lines of:
```
clang-14 and gcc-11 fixed this.
```
or w/e. This helps us find (via grep) and remove cruft when the
minimum supported compiler versions are updated.
Note: gcc-10 had a bug with the symbolic references to labels when
using tied constraints.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98096
Both compilers had bugs here, and it may be worth mentioning that in
the commit message.
>
> [1] https://github.com/ClangBuiltLinux/linux/issues/1512
> [2] https://lore.kernel.org/all/YfMruK8%2F1izZ2VHS@google.com
>
> Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> init/Kconfig | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index e9119bf54b1f..a206b21703be 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -77,6 +77,10 @@ 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
> + 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
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] Kconfig: Add option for asm goto w/ tied outputs to workaround clang-13 bug
2022-02-01 20:16 ` Nick Desaulniers
@ 2022-02-01 20:56 ` Sean Christopherson
2022-02-01 21:15 ` Nick Desaulniers
0 siblings, 1 reply; 10+ messages in thread
From: Sean Christopherson @ 2022-02-01 20:56 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Paolo Bonzini, Nathan Chancellor, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel, kvm, llvm, linux-kernel,
Peter Zijlstra, syzbot+6cde2282daa792c49ab8
On Tue, Feb 01, 2022, Nick Desaulniers wrote:
> On Mon, Jan 31, 2022 at 5:08 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > 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.
>
> LGTM.
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
>
> If you're going to respin the series, consider adding a comment in the
> source along the lines of:
> ```
> clang-14 and gcc-11 fixed this.
> ```
> or w/e. This helps us find (via grep) and remove cruft when the
> minimum supported compiler versions are updated.
Will do, a new version is definitely needed.
> Note: gcc-10 had a bug with the symbolic references to labels when
> using tied constraints.
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98096
>
> Both compilers had bugs here, and it may be worth mentioning that in
> the commit message.
Is this wording accurate?
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] Kconfig: Add option for asm goto w/ tied outputs to workaround clang-13 bug
2022-02-01 20:56 ` Sean Christopherson
@ 2022-02-01 21:15 ` Nick Desaulniers
0 siblings, 0 replies; 10+ messages in thread
From: Nick Desaulniers @ 2022-02-01 21:15 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Nathan Chancellor, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel, kvm, llvm, linux-kernel,
Peter Zijlstra, syzbot+6cde2282daa792c49ab8
On Tue, Feb 1, 2022 at 12:56 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Feb 01, 2022, Nick Desaulniers wrote:
> > Note: gcc-10 had a bug with the symbolic references to labels when
> > using tied constraints.
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98096
> >
> > Both compilers had bugs here, and it may be worth mentioning that in
> > the commit message.
>
> Is this wording accurate?
>
> 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.
SGTM
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/5] x86/uaccess: Implement macros for CMPXCHG on user addresses
2022-02-01 1:08 [PATCH 0/5] x86: uaccess CMPXCHG + KVM bug fixes Sean Christopherson
2022-02-01 1:08 ` [PATCH 1/5] Kconfig: Add option for asm goto w/ tied outputs to workaround clang-13 bug Sean Christopherson
@ 2022-02-01 1:08 ` Sean Christopherson
2022-02-01 1:08 ` [PATCH 3/5] KVM: x86: Use __try_cmpxchg_user() to update guest PTE A/D bits Sean Christopherson
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2022-02-01 1:08 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,
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 | 131 +++++++++++++++++++++++++++++++++
1 file changed, 131 insertions(+)
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index ac96f9b2d64b..423bfcc1ec4b 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -409,6 +409,98 @@ 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
+#define __try_cmpxchg64_user_asm(_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 "cmpxchg8b %[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), \
+ "+A" (__old), \
+ [ptr] "+m" (*_ptr) \
+ : "b" ((u32)__new), \
+ "c" ((u32)((u64)__new >> 32)) \
+ : "memory", "cc"); \
+ if (unlikely(__err)) \
+ goto label; \
+ if (unlikely(!success)) \
+ *_old = __old; \
+ likely(success); })
+#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 +593,45 @@ 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
+
+#define unsafe_try_cmpxchg_user(_ptr, _oldp, _nval, _label) ({ \
+ bool __ret; \
+ switch (sizeof(*(_ptr))) { \
+ case 1: __ret = __try_cmpxchg_user_asm("b", "q", \
+ (_ptr), (_oldp), \
+ (_nval), _label); \
+ break; \
+ case 2: __ret = __try_cmpxchg_user_asm("w", "r", \
+ (_ptr), (_oldp), \
+ (_nval), _label); \
+ break; \
+ case 4: __ret = __try_cmpxchg_user_asm("l", "r", \
+ (_ptr), (_oldp), \
+ (_nval), _label); \
+ break; \
+ case 8: __ret = __try_cmpxchg64_user_asm((_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] 10+ messages in thread
* [PATCH 3/5] KVM: x86: Use __try_cmpxchg_user() to update guest PTE A/D bits
2022-02-01 1:08 [PATCH 0/5] x86: uaccess CMPXCHG + KVM bug fixes Sean Christopherson
2022-02-01 1:08 ` [PATCH 1/5] Kconfig: Add option for asm goto w/ tied outputs to workaround clang-13 bug Sean Christopherson
2022-02-01 1:08 ` [PATCH 2/5] x86/uaccess: Implement macros for CMPXCHG on user addresses Sean Christopherson
@ 2022-02-01 1:08 ` Sean Christopherson
2022-02-01 1:08 ` [PATCH 4/5] KVM: x86: Use __try_cmpxchg_user() to emulate atomic accesses Sean Christopherson
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2022-02-01 1:08 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,
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>
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] 10+ messages in thread
* [PATCH 4/5] KVM: x86: Use __try_cmpxchg_user() to emulate atomic accesses
2022-02-01 1:08 [PATCH 0/5] x86: uaccess CMPXCHG + KVM bug fixes Sean Christopherson
` (2 preceding siblings ...)
2022-02-01 1:08 ` [PATCH 3/5] KVM: x86: Use __try_cmpxchg_user() to update guest PTE A/D bits Sean Christopherson
@ 2022-02-01 1:08 ` Sean Christopherson
2022-02-01 1:08 ` [PATCH 5/5] KVM: x86: Bail to userspace if emulation of atomic user access faults Sean Christopherson
2022-02-01 17:09 ` [PATCH 0/5] x86: uaccess CMPXCHG + KVM bug fixes Tadeusz Struk
5 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2022-02-01 1:08 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,
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..37064d565bbc 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 *)(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] 10+ messages in thread
* [PATCH 5/5] KVM: x86: Bail to userspace if emulation of atomic user access faults
2022-02-01 1:08 [PATCH 0/5] x86: uaccess CMPXCHG + KVM bug fixes Sean Christopherson
` (3 preceding siblings ...)
2022-02-01 1:08 ` [PATCH 4/5] KVM: x86: Use __try_cmpxchg_user() to emulate atomic accesses Sean Christopherson
@ 2022-02-01 1:08 ` Sean Christopherson
2022-02-01 17:09 ` [PATCH 0/5] x86: uaccess CMPXCHG + KVM bug fixes Tadeusz Struk
5 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2022-02-01 1:08 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,
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 37064d565bbc..66c5410dd4c3 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] 10+ messages in thread
* Re: [PATCH 0/5] x86: uaccess CMPXCHG + KVM bug fixes
2022-02-01 1:08 [PATCH 0/5] x86: uaccess CMPXCHG + KVM bug fixes Sean Christopherson
` (4 preceding siblings ...)
2022-02-01 1:08 ` [PATCH 5/5] KVM: x86: Bail to userspace if emulation of atomic user access faults Sean Christopherson
@ 2022-02-01 17:09 ` Tadeusz Struk
5 siblings, 0 replies; 10+ messages in thread
From: Tadeusz Struk @ 2022-02-01 17:09 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini, Nathan Chancellor, Nick Desaulniers
Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
llvm, linux-kernel, Peter Zijlstra, syzbot+6cde2282daa792c49ab8,
Tadeusz Struk
On 1/31/22 17:08, Sean Christopherson wrote:
> Add uaccess macros for doing CMPXCHG on userspace addresses and use the
> macros to fix KVM bugs by replacing flawed code that maps memory into the
> kernel address space without proper mmu_notifier protection (or with
> broken pfn calculations in one case).
>
> Add yet another Kconfig for guarding asm_volatile_goto() to workaround a
> clang-13 bug. I've verified the test passes on gcc versions of arm64,
> PPC, RISC-V, and s390x that also pass the CC_HAS_ASM_GOTO_OUTPUT test.
>
> Patches 1-4 are tagged for stable@ as patches 3 and 4 (mostly 3) need a
> backportable fix, and doing CMPXCHG on the userspace address is the
> simplest fix from a KVM perspective.
>
> Peter Zijlstra (1):
> x86/uaccess: Implement macros for CMPXCHG on user addresses
>
> Sean Christopherson (4):
> Kconfig: Add option for asm goto w/ tied outputs to workaround
> clang-13 bug
> KVM: x86: Use __try_cmpxchg_user() to update guest PTE A/D bits
> KVM: x86: Use __try_cmpxchg_user() to emulate atomic accesses
> KVM: x86: Bail to userspace if emulation of atomic user access faults
>
> arch/x86/include/asm/uaccess.h | 131 +++++++++++++++++++++++++++++++++
> arch/x86/kvm/mmu/paging_tmpl.h | 45 +----------
> arch/x86/kvm/x86.c | 35 ++++-----
> init/Kconfig | 4 +
> 4 files changed, 150 insertions(+), 65 deletions(-)
This also fixes the following syzbot issue:
https://syzkaller.appspot.com/bug?id=6cb6102a0a7b0c52060753dd62d070a1d1e71347
Tested-by: Tadeusz Struk <tadeusz.struk@linaro.org>
--
Thanks,
Tadeusz
^ permalink raw reply [flat|nested] 10+ messages in thread