A simple fix for a null pointer dereference in vmx_vcpu_run() with an ugly-but-safe prereq patch.
The fix also has a wart/hack where it marks RSP as clobbered using ASM_CALL_CONSTRAINT to workaround an issue where the VM-Exit label isn't found by _something_ during modpost. I vaguely recall seeing the same issue when I first worked on this code a few years back. I think it was objtool that was confused, but I can't remember the details for the life of me. I don't have more cycles to throw at deciphering the thing, and marking RSP as clobbered is safe, so I went with the hack.
Alternatively, reverting the offending commit (added in v4.19.119) would fix the immediate issue, but RDX and RSI technically need to be marked as clobbered even though it's extremely unlikely the compiler will consume their bad value. All of the above ugliness seems preferable to leaving a known bug in place.
Sean Christopherson (2): KVM: VMX: Explicitly reference RCX as the vmx_vcpu pointer in asm blobs KVM: VMX: Mark RCX, RDX and RSI as clobbered in vmx_vcpu_run()'s asm blob
arch/x86/kvm/vmx.c | 89 +++++++++++++++++++++++++--------------------- 1 file changed, 49 insertions(+), 40 deletions(-)
Upstream commit 051a2d3e59e51ae49fd56aef34e472832897ce46.
Use '%% " _ASM_CX"' instead of '%0' to dereference RCX, i.e. the 'struct vcpu_vmx' pointer, in the VM-Enter asm blobs of vmx_vcpu_run() and nested_vmx_check_vmentry_hw(). Using the symbolic name means that adding/removing an output parameter(s) requires "rewriting" almost all of the asm blob, which makes it nearly impossible to understand what's being changed in even the most minor patches.
Opportunistically improve the code comments.
Signed-off-by: Sean Christopherson sean.j.christopherson@intel.com --- arch/x86/kvm/vmx.c | 86 +++++++++++++++++++++++++--------------------- 1 file changed, 47 insertions(+), 39 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index fe5036641c59..5b06a98ffd4c 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -10776,9 +10776,9 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu) "push %%" _ASM_DX "; push %%" _ASM_BP ";" "push %%" _ASM_CX " \n\t" /* placeholder for guest rcx */ "push %%" _ASM_CX " \n\t" - "cmp %%" _ASM_SP ", %c[host_rsp](%0) \n\t" + "cmp %%" _ASM_SP ", %c[host_rsp](%%" _ASM_CX ") \n\t" "je 1f \n\t" - "mov %%" _ASM_SP ", %c[host_rsp](%0) \n\t" + "mov %%" _ASM_SP ", %c[host_rsp](%%" _ASM_CX ") \n\t" /* Avoid VMWRITE when Enlightened VMCS is in use */ "test %%" _ASM_SI ", %%" _ASM_SI " \n\t" "jz 2f \n\t" @@ -10788,32 +10788,33 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu) __ex(ASM_VMX_VMWRITE_RSP_RDX) "\n\t" "1: \n\t" /* Reload cr2 if changed */ - "mov %c[cr2](%0), %%" _ASM_AX " \n\t" + "mov %c[cr2](%%" _ASM_CX "), %%" _ASM_AX " \n\t" "mov %%cr2, %%" _ASM_DX " \n\t" "cmp %%" _ASM_AX ", %%" _ASM_DX " \n\t" "je 3f \n\t" "mov %%" _ASM_AX", %%cr2 \n\t" "3: \n\t" /* Check if vmlaunch of vmresume is needed */ - "cmpb $0, %c[launched](%0) \n\t" + "cmpb $0, %c[launched](%%" _ASM_CX ") \n\t" /* Load guest registers. Don't clobber flags. */ - "mov %c[rax](%0), %%" _ASM_AX " \n\t" - "mov %c[rbx](%0), %%" _ASM_BX " \n\t" - "mov %c[rdx](%0), %%" _ASM_DX " \n\t" - "mov %c[rsi](%0), %%" _ASM_SI " \n\t" - "mov %c[rdi](%0), %%" _ASM_DI " \n\t" - "mov %c[rbp](%0), %%" _ASM_BP " \n\t" + "mov %c[rax](%%" _ASM_CX "), %%" _ASM_AX " \n\t" + "mov %c[rbx](%%" _ASM_CX "), %%" _ASM_BX " \n\t" + "mov %c[rdx](%%" _ASM_CX "), %%" _ASM_DX " \n\t" + "mov %c[rsi](%%" _ASM_CX "), %%" _ASM_SI " \n\t" + "mov %c[rdi](%%" _ASM_CX "), %%" _ASM_DI " \n\t" + "mov %c[rbp](%%" _ASM_CX "), %%" _ASM_BP " \n\t" #ifdef CONFIG_X86_64 - "mov %c[r8](%0), %%r8 \n\t" - "mov %c[r9](%0), %%r9 \n\t" - "mov %c[r10](%0), %%r10 \n\t" - "mov %c[r11](%0), %%r11 \n\t" - "mov %c[r12](%0), %%r12 \n\t" - "mov %c[r13](%0), %%r13 \n\t" - "mov %c[r14](%0), %%r14 \n\t" - "mov %c[r15](%0), %%r15 \n\t" + "mov %c[r8](%%" _ASM_CX "), %%r8 \n\t" + "mov %c[r9](%%" _ASM_CX "), %%r9 \n\t" + "mov %c[r10](%%" _ASM_CX "), %%r10 \n\t" + "mov %c[r11](%%" _ASM_CX "), %%r11 \n\t" + "mov %c[r12](%%" _ASM_CX "), %%r12 \n\t" + "mov %c[r13](%%" _ASM_CX "), %%r13 \n\t" + "mov %c[r14](%%" _ASM_CX "), %%r14 \n\t" + "mov %c[r15](%%" _ASM_CX "), %%r15 \n\t" #endif - "mov %c[rcx](%0), %%" _ASM_CX " \n\t" /* kills %0 (ecx) */ + /* Load guest RCX. This kills the vmx_vcpu pointer! */ + "mov %c[rcx](%%" _ASM_CX "), %%" _ASM_CX " \n\t"
/* Enter guest mode */ "jne 1f \n\t" @@ -10821,26 +10822,33 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu) "jmp 2f \n\t" "1: " __ex(ASM_VMX_VMRESUME) "\n\t" "2: " - /* Save guest registers, load host registers, keep flags */ - "mov %0, %c[wordsize](%%" _ASM_SP ") \n\t" - "pop %0 \n\t" - "setbe %c[fail](%0)\n\t" - "mov %%" _ASM_AX ", %c[rax](%0) \n\t" - "mov %%" _ASM_BX ", %c[rbx](%0) \n\t" - __ASM_SIZE(pop) " %c[rcx](%0) \n\t" - "mov %%" _ASM_DX ", %c[rdx](%0) \n\t" - "mov %%" _ASM_SI ", %c[rsi](%0) \n\t" - "mov %%" _ASM_DI ", %c[rdi](%0) \n\t" - "mov %%" _ASM_BP ", %c[rbp](%0) \n\t" + + /* Save guest's RCX to the stack placeholder (see above) */ + "mov %%" _ASM_CX ", %c[wordsize](%%" _ASM_SP ") \n\t" + + /* Load host's RCX, i.e. the vmx_vcpu pointer */ + "pop %%" _ASM_CX " \n\t" + + /* Set vmx->fail based on EFLAGS.{CF,ZF} */ + "setbe %c[fail](%%" _ASM_CX ")\n\t" + + /* Save all guest registers, including RCX from the stack */ + "mov %%" _ASM_AX ", %c[rax](%%" _ASM_CX ") \n\t" + "mov %%" _ASM_BX ", %c[rbx](%%" _ASM_CX ") \n\t" + __ASM_SIZE(pop) " %c[rcx](%%" _ASM_CX ") \n\t" + "mov %%" _ASM_DX ", %c[rdx](%%" _ASM_CX ") \n\t" + "mov %%" _ASM_SI ", %c[rsi](%%" _ASM_CX ") \n\t" + "mov %%" _ASM_DI ", %c[rdi](%%" _ASM_CX ") \n\t" + "mov %%" _ASM_BP ", %c[rbp](%%" _ASM_CX ") \n\t" #ifdef CONFIG_X86_64 - "mov %%r8, %c[r8](%0) \n\t" - "mov %%r9, %c[r9](%0) \n\t" - "mov %%r10, %c[r10](%0) \n\t" - "mov %%r11, %c[r11](%0) \n\t" - "mov %%r12, %c[r12](%0) \n\t" - "mov %%r13, %c[r13](%0) \n\t" - "mov %%r14, %c[r14](%0) \n\t" - "mov %%r15, %c[r15](%0) \n\t" + "mov %%r8, %c[r8](%%" _ASM_CX ") \n\t" + "mov %%r9, %c[r9](%%" _ASM_CX ") \n\t" + "mov %%r10, %c[r10](%%" _ASM_CX ") \n\t" + "mov %%r11, %c[r11](%%" _ASM_CX ") \n\t" + "mov %%r12, %c[r12](%%" _ASM_CX ") \n\t" + "mov %%r13, %c[r13](%%" _ASM_CX ") \n\t" + "mov %%r14, %c[r14](%%" _ASM_CX ") \n\t" + "mov %%r15, %c[r15](%%" _ASM_CX ") \n\t"
/* * Clear all general purpose registers (except RSP, which is loaded by @@ -10860,7 +10868,7 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu) "xor %%r15d, %%r15d \n\t" #endif "mov %%cr2, %%" _ASM_AX " \n\t" - "mov %%" _ASM_AX ", %c[cr2](%0) \n\t" + "mov %%" _ASM_AX ", %c[cr2](%%" _ASM_CX ") \n\t"
"xor %%eax, %%eax \n\t" "xor %%ebx, %%ebx \n\t"
Save RCX, RDX and RSI to fake outputs to coerce the compiler into treating them as clobbered. RCX in particular is likely to be reused by the compiler to dereference the 'struct vcpu_vmx' pointer, which will result in a null pointer dereference now that RCX is zeroed by the asm blob.
Add ASM_CALL_CONSTRAINT to fudge around an issue where <something> during modpost can't find vmx_return when specifying output constraints.
Reported-by: Tobias Urdin tobias.urdin@binero.com Fixes: b4be98039a92 ("KVM: VMX: Zero out *all* general purpose registers after VM-Exit") Signed-off-by: Sean Christopherson sean.j.christopherson@intel.com --- arch/x86/kvm/vmx.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 5b06a98ffd4c..54c8b4dc750d 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -10882,7 +10882,8 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu) ".global vmx_return \n\t" "vmx_return: " _ASM_PTR " 2b \n\t" ".popsection" - : : "c"(vmx), "d"((unsigned long)HOST_RSP), "S"(evmcs_rsp), + : ASM_CALL_CONSTRAINT, "=c"((int){0}), "=d"((int){0}), "=S"((int){0}) + : "c"(vmx), "d"((unsigned long)HOST_RSP), "S"(evmcs_rsp), [launched]"i"(offsetof(struct vcpu_vmx, __launched)), [fail]"i"(offsetof(struct vcpu_vmx, fail)), [host_rsp]"i"(offsetof(struct vcpu_vmx, host_rsp)),
On Mon, May 04, 2020 at 06:23:48PM -0700, Sean Christopherson wrote:
Save RCX, RDX and RSI to fake outputs to coerce the compiler into treating them as clobbered. RCX in particular is likely to be reused by the compiler to dereference the 'struct vcpu_vmx' pointer, which will result in a null pointer dereference now that RCX is zeroed by the asm blob.
Add ASM_CALL_CONSTRAINT to fudge around an issue where <something> during modpost can't find vmx_return when specifying output constraints.
Reported-by: Tobias Urdin tobias.urdin@binero.com Fixes: b4be98039a92 ("KVM: VMX: Zero out *all* general purpose registers after VM-Exit") Signed-off-by: Sean Christopherson sean.j.christopherson@intel.com
arch/x86/kvm/vmx.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 5b06a98ffd4c..54c8b4dc750d 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -10882,7 +10882,8 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu) ".global vmx_return \n\t" "vmx_return: " _ASM_PTR " 2b \n\t" ".popsection"
: : "c"(vmx), "d"((unsigned long)HOST_RSP), "S"(evmcs_rsp),
: ASM_CALL_CONSTRAINT, "=c"((int){0}), "=d"((int){0}), "=S"((int){0}): "c"(vmx), "d"((unsigned long)HOST_RSP), "S"(evmcs_rsp),-- 2.26.0
What is the git commit id of this patch in Linus's tree?
thanks,
greg k-h
On Tue, May 05, 2020 at 08:15:02AM +0200, Greg Kroah-Hartman wrote:
On Mon, May 04, 2020 at 06:23:48PM -0700, Sean Christopherson wrote:
Save RCX, RDX and RSI to fake outputs to coerce the compiler into treating them as clobbered. RCX in particular is likely to be reused by the compiler to dereference the 'struct vcpu_vmx' pointer, which will result in a null pointer dereference now that RCX is zeroed by the asm blob.
Add ASM_CALL_CONSTRAINT to fudge around an issue where <something> during modpost can't find vmx_return when specifying output constraints.
Reported-by: Tobias Urdin tobias.urdin@binero.com Fixes: b4be98039a92 ("KVM: VMX: Zero out *all* general purpose registers after VM-Exit") Signed-off-by: Sean Christopherson sean.j.christopherson@intel.com
arch/x86/kvm/vmx.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 5b06a98ffd4c..54c8b4dc750d 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -10882,7 +10882,8 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu) ".global vmx_return \n\t" "vmx_return: " _ASM_PTR " 2b \n\t" ".popsection"
: : "c"(vmx), "d"((unsigned long)HOST_RSP), "S"(evmcs_rsp),
: ASM_CALL_CONSTRAINT, "=c"((int){0}), "=d"((int){0}), "=S"((int){0}): "c"(vmx), "d"((unsigned long)HOST_RSP), "S"(evmcs_rsp),-- 2.26.0
What is the git commit id of this patch in Linus's tree?
There is none. In upstream at the time of the offending commit (b4be98039a92 in 4.19, 0e0ab73c9a024 upstream), the inline asm blob had previously been moved to a dedicated helper, __vmx_vcpu_run(), that was intentionally put into a separate compilation unit, i.e. consuming the clobbered register was effectively impossible because %rcx is volatile and __vmx_vcpu_run() couldn't itself be inlined.
To make things more confusing, the inline asm blob got moved into a proper asm subroutine shortly thereafter. Things really start to diverge from current upstream right around the time of this commit.
On Mon, May 04, 2020 at 11:27:31PM -0700, Sean Christopherson wrote:
On Tue, May 05, 2020 at 08:15:02AM +0200, Greg Kroah-Hartman wrote:
On Mon, May 04, 2020 at 06:23:48PM -0700, Sean Christopherson wrote:
Save RCX, RDX and RSI to fake outputs to coerce the compiler into treating them as clobbered. RCX in particular is likely to be reused by the compiler to dereference the 'struct vcpu_vmx' pointer, which will result in a null pointer dereference now that RCX is zeroed by the asm blob.
Add ASM_CALL_CONSTRAINT to fudge around an issue where <something> during modpost can't find vmx_return when specifying output constraints.
Reported-by: Tobias Urdin tobias.urdin@binero.com Fixes: b4be98039a92 ("KVM: VMX: Zero out *all* general purpose registers after VM-Exit") Signed-off-by: Sean Christopherson sean.j.christopherson@intel.com
arch/x86/kvm/vmx.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 5b06a98ffd4c..54c8b4dc750d 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -10882,7 +10882,8 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu) ".global vmx_return \n\t" "vmx_return: " _ASM_PTR " 2b \n\t" ".popsection"
: : "c"(vmx), "d"((unsigned long)HOST_RSP), "S"(evmcs_rsp),
: ASM_CALL_CONSTRAINT, "=c"((int){0}), "=d"((int){0}), "=S"((int){0}): "c"(vmx), "d"((unsigned long)HOST_RSP), "S"(evmcs_rsp),-- 2.26.0
What is the git commit id of this patch in Linus's tree?
There is none. In upstream at the time of the offending commit (b4be98039a92 in 4.19, 0e0ab73c9a024 upstream), the inline asm blob had previously been moved to a dedicated helper, __vmx_vcpu_run(), that was intentionally put into a separate compilation unit, i.e. consuming the clobbered register was effectively impossible because %rcx is volatile and __vmx_vcpu_run() couldn't itself be inlined.
To make things more confusing, the inline asm blob got moved into a proper asm subroutine shortly thereafter. Things really start to diverge from current upstream right around the time of this commit.
Then you need to document the heck out of the fact that this is not upstream, why it is different from upstream, and why we can't just take what upstream did instead in the changelog. That way, when this patch turns out to be buggy (hint, 90% of the times they are), we know why this was done the way it was so we can revert it and know who to complain to :)
thanks,
greg k-h
On Tue, May 05, 2020 at 09:02:59AM +0200, Greg Kroah-Hartman wrote:
On Mon, May 04, 2020 at 11:27:31PM -0700, Sean Christopherson wrote:
On Tue, May 05, 2020 at 08:15:02AM +0200, Greg Kroah-Hartman wrote:
On Mon, May 04, 2020 at 06:23:48PM -0700, Sean Christopherson wrote:
Save RCX, RDX and RSI to fake outputs to coerce the compiler into treating them as clobbered. RCX in particular is likely to be reused by the compiler to dereference the 'struct vcpu_vmx' pointer, which will result in a null pointer dereference now that RCX is zeroed by the asm blob.
Add ASM_CALL_CONSTRAINT to fudge around an issue where <something> during modpost can't find vmx_return when specifying output constraints.
Reported-by: Tobias Urdin tobias.urdin@binero.com Fixes: b4be98039a92 ("KVM: VMX: Zero out *all* general purpose registers after VM-Exit") Signed-off-by: Sean Christopherson sean.j.christopherson@intel.com
arch/x86/kvm/vmx.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 5b06a98ffd4c..54c8b4dc750d 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -10882,7 +10882,8 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu) ".global vmx_return \n\t" "vmx_return: " _ASM_PTR " 2b \n\t" ".popsection"
: : "c"(vmx), "d"((unsigned long)HOST_RSP), "S"(evmcs_rsp),
: ASM_CALL_CONSTRAINT, "=c"((int){0}), "=d"((int){0}), "=S"((int){0}): "c"(vmx), "d"((unsigned long)HOST_RSP), "S"(evmcs_rsp),-- 2.26.0
What is the git commit id of this patch in Linus's tree?
There is none. In upstream at the time of the offending commit (b4be98039a92 in 4.19, 0e0ab73c9a024 upstream), the inline asm blob had previously been moved to a dedicated helper, __vmx_vcpu_run(), that was intentionally put into a separate compilation unit, i.e. consuming the clobbered register was effectively impossible because %rcx is volatile and __vmx_vcpu_run() couldn't itself be inlined.
To make things more confusing, the inline asm blob got moved into a proper asm subroutine shortly thereafter. Things really start to diverge from current upstream right around the time of this commit.
Then you need to document the heck out of the fact that this is not upstream, why it is different from upstream, and why we can't just take what upstream did instead in the changelog. That way, when this patch turns out to be buggy (hint, 90% of the times they are), we know why this was done the way it was so we can revert it and know who to complain to :)
Will do.
linux-stable-mirror@lists.linaro.org
-
Greg Kroah-Hartman -
Sean Christopherson