All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 4.19 STABLE 0/2] KVM: VMX: Fix null pointer dereference
@ 2020-05-05  1:23 Sean Christopherson
  2020-05-05  1:23 ` [PATCH 4.19 STABLE 1/2] KVM: VMX: Explicitly reference RCX as the vmx_vcpu pointer in asm blobs Sean Christopherson
  2020-05-05  1:23 ` [PATCH 4.19 STABLE 2/2] KVM: VMX: Mark RCX, RDX and RSI as clobbered in vmx_vcpu_run()'s asm blob Sean Christopherson
  0 siblings, 2 replies; 7+ messages in thread
From: Sean Christopherson @ 2020-05-05  1:23 UTC (permalink / raw)
  To: stable, Greg Kroah-Hartman, Ben Hutchings, Sasha Levin
  Cc: Paolo Bonzini, linux-kernel, Tobias Urdin

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(-)

-- 
2.26.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 4.19 STABLE 1/2] KVM: VMX: Explicitly reference RCX as the vmx_vcpu pointer in asm blobs
  2020-05-05  1:23 [PATCH 4.19 STABLE 0/2] KVM: VMX: Fix null pointer dereference Sean Christopherson
@ 2020-05-05  1:23 ` Sean Christopherson
  2020-05-05  1:23 ` [PATCH 4.19 STABLE 2/2] KVM: VMX: Mark RCX, RDX and RSI as clobbered in vmx_vcpu_run()'s asm blob Sean Christopherson
  1 sibling, 0 replies; 7+ messages in thread
From: Sean Christopherson @ 2020-05-05  1:23 UTC (permalink / raw)
  To: stable, Greg Kroah-Hartman, Ben Hutchings, Sasha Levin
  Cc: Paolo Bonzini, linux-kernel, Tobias Urdin

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"
-- 
2.26.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 4.19 STABLE 2/2] KVM: VMX: Mark RCX, RDX and RSI as clobbered in vmx_vcpu_run()'s asm blob
  2020-05-05  1:23 [PATCH 4.19 STABLE 0/2] KVM: VMX: Fix null pointer dereference Sean Christopherson
  2020-05-05  1:23 ` [PATCH 4.19 STABLE 1/2] KVM: VMX: Explicitly reference RCX as the vmx_vcpu pointer in asm blobs Sean Christopherson
@ 2020-05-05  1:23 ` Sean Christopherson
  2020-05-05  6:15   ` Greg Kroah-Hartman
  1 sibling, 1 reply; 7+ messages in thread
From: Sean Christopherson @ 2020-05-05  1:23 UTC (permalink / raw)
  To: stable, Greg Kroah-Hartman, Ben Hutchings, Sasha Levin
  Cc: Paolo Bonzini, linux-kernel, Tobias Urdin

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)),
-- 
2.26.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 4.19 STABLE 2/2] KVM: VMX: Mark RCX, RDX and RSI as clobbered in vmx_vcpu_run()'s asm blob
  2020-05-05  1:23 ` [PATCH 4.19 STABLE 2/2] KVM: VMX: Mark RCX, RDX and RSI as clobbered in vmx_vcpu_run()'s asm blob Sean Christopherson
@ 2020-05-05  6:15   ` Greg Kroah-Hartman
  2020-05-05  6:27     ` Sean Christopherson
  0 siblings, 1 reply; 7+ messages in thread
From: Greg Kroah-Hartman @ 2020-05-05  6:15 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: stable, Ben Hutchings, Sasha Levin, Paolo Bonzini, linux-kernel,
	Tobias Urdin

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),
>  		[launched]"i"(offsetof(struct vcpu_vmx, __launched)),
>  		[fail]"i"(offsetof(struct vcpu_vmx, fail)),
>  		[host_rsp]"i"(offsetof(struct vcpu_vmx, host_rsp)),
> -- 
> 2.26.0
> 

What is the git commit id of this patch in Linus's tree?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 4.19 STABLE 2/2] KVM: VMX: Mark RCX, RDX and RSI as clobbered in vmx_vcpu_run()'s asm blob
  2020-05-05  6:15   ` Greg Kroah-Hartman
@ 2020-05-05  6:27     ` Sean Christopherson
  2020-05-05  7:02       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 7+ messages in thread
From: Sean Christopherson @ 2020-05-05  6:27 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: stable, Ben Hutchings, Sasha Levin, Paolo Bonzini, linux-kernel,
	Tobias Urdin

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),
> >  		[launched]"i"(offsetof(struct vcpu_vmx, __launched)),
> >  		[fail]"i"(offsetof(struct vcpu_vmx, fail)),
> >  		[host_rsp]"i"(offsetof(struct vcpu_vmx, host_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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 4.19 STABLE 2/2] KVM: VMX: Mark RCX, RDX and RSI as clobbered in vmx_vcpu_run()'s asm blob
  2020-05-05  6:27     ` Sean Christopherson
@ 2020-05-05  7:02       ` Greg Kroah-Hartman
  2020-05-05 14:37         ` Sean Christopherson
  0 siblings, 1 reply; 7+ messages in thread
From: Greg Kroah-Hartman @ 2020-05-05  7:02 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: stable, Ben Hutchings, Sasha Levin, Paolo Bonzini, linux-kernel,
	Tobias Urdin

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),
> > >  		[launched]"i"(offsetof(struct vcpu_vmx, __launched)),
> > >  		[fail]"i"(offsetof(struct vcpu_vmx, fail)),
> > >  		[host_rsp]"i"(offsetof(struct vcpu_vmx, host_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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 4.19 STABLE 2/2] KVM: VMX: Mark RCX, RDX and RSI as clobbered in vmx_vcpu_run()'s asm blob
  2020-05-05  7:02       ` Greg Kroah-Hartman
@ 2020-05-05 14:37         ` Sean Christopherson
  0 siblings, 0 replies; 7+ messages in thread
From: Sean Christopherson @ 2020-05-05 14:37 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: stable, Ben Hutchings, Sasha Levin, Paolo Bonzini, linux-kernel,
	Tobias Urdin

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),
> > > >  		[launched]"i"(offsetof(struct vcpu_vmx, __launched)),
> > > >  		[fail]"i"(offsetof(struct vcpu_vmx, fail)),
> > > >  		[host_rsp]"i"(offsetof(struct vcpu_vmx, host_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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-05-05 14:37 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-05  1:23 [PATCH 4.19 STABLE 0/2] KVM: VMX: Fix null pointer dereference Sean Christopherson
2020-05-05  1:23 ` [PATCH 4.19 STABLE 1/2] KVM: VMX: Explicitly reference RCX as the vmx_vcpu pointer in asm blobs Sean Christopherson
2020-05-05  1:23 ` [PATCH 4.19 STABLE 2/2] KVM: VMX: Mark RCX, RDX and RSI as clobbered in vmx_vcpu_run()'s asm blob Sean Christopherson
2020-05-05  6:15   ` Greg Kroah-Hartman
2020-05-05  6:27     ` Sean Christopherson
2020-05-05  7:02       ` Greg Kroah-Hartman
2020-05-05 14:37         ` Sean Christopherson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.