public inbox for kvm@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] KVM: nVMX: Fix bug of injecting L2 exception into L1
@ 2017-11-19 16:25 Liran Alon
  2017-11-20 21:47 ` Paolo Bonzini
  2018-01-09 10:08 ` Paolo Bonzini
  0 siblings, 2 replies; 6+ messages in thread
From: Liran Alon @ 2017-11-19 16:25 UTC (permalink / raw)
  To: pbonzini, rkrcmar, kvm
  Cc: jmattson, wanpeng.li, idan.brown, Liran Alon, Krish Sadhukhan

kvm_clear_exception_queue() should clear pending exception.
This also includes exceptions which were only marked pending but not
yet injected. This is because exception.pending is used for both L1
and L2 to determine if an exception should be raised to guest.
Note that an exception which is pending but not yet injected will
be raised again once the guest will be resumed.

Consider the following scenario:
1) L0 KVM with ignore_msrs=false.
2) L1 prepare vmcs12 with the following:
    a) No intercepts on MSR (MSR_BITMAP exist and is filled with 0).
    b) No intercept for #GP.
    c) vmx-preemption-timer is configured.
3) L1 enters into L2.
4) L2 reads an unhandled MSR that exists in MSR_BITMAP
(such as 0x1fff).

L2 RDMSR could be handled as described below:
1) L2 exits to L0 on RDMSR and calls handle_rdmsr().
2) handle_rdmsr() calls kvm_inject_gp() which sets
KVM_REQ_EVENT, exception.pending=true and exception.injected=false.
3) vcpu_enter_guest() consumes KVM_REQ_EVENT and calls
inject_pending_event() which calls vmx_check_nested_events()
which sees that exception.pending=true but
nested_vmx_check_exception() returns 0 and therefore does nothing at
this point. However let's assume it later sees vmx-preemption-timer
expired and therefore exits from L2 to L1 by calling
nested_vmx_vmexit().
4) nested_vmx_vmexit() calls prepare_vmcs12()
which calls vmcs12_save_pending_event() but it does nothing as
exception.injected is false. Also prepare_vmcs12() calls
kvm_clear_exception_queue() which does nothing as
exception.injected is already false.
5) We now return from vmx_check_nested_events() with 0 while still
having exception.pending=true!
6) Therefore inject_pending_event() continues
and we inject L2 exception to L1!...

This commit will fix above issue by changing step (4) to
clear exception.pending in kvm_clear_exception_queue().

Fixes: 664f8e26b00c ("KVM: X86: Fix loss of exception which has not
yet been injected")

Signed-off-by: Liran Alon <liran.alon@oracle.com>
Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 arch/x86/kvm/vmx.c | 1 -
 arch/x86/kvm/x86.h | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 7c3522a989d0..bee08782c781 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -11035,7 +11035,6 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu, bool external_intr)
 		if (vmx->nested.nested_run_pending)
 			return -EBUSY;
 		nested_vmx_inject_exception_vmexit(vcpu, exit_qual);
-		vcpu->arch.exception.pending = false;
 		return 0;
 	}
 
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index d0b95b7a90b4..6d112d8f799c 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -12,6 +12,7 @@
 
 static inline void kvm_clear_exception_queue(struct kvm_vcpu *vcpu)
 {
+	vcpu->arch.exception.pending = false;
 	vcpu->arch.exception.injected = false;
 }
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: nVMX: Fix bug of injecting L2 exception into L1
@ 2017-12-12  0:06 Liran Alon
  0 siblings, 0 replies; 6+ messages in thread
From: Liran Alon @ 2017-12-12  0:06 UTC (permalink / raw)
  To: pbonzini; +Cc: jmattson, krish.sadhukhan, rkrcmar, wanpeng.li, idan.brown, kvm


On 21/11/17 00:58, Paolo Bonzini wrote:
> On 20/11/2017 23:46, Liran Alon wrote:
>>>>
>>>
>>> But this is buggy as well, because the #GP is lost, isn't it?
>>
>> I don't think so.
>>
>> Since commit 664f8e26b00c ("KVM: X86: Fix loss of exception which has
>> not yet been injected"), there is a fundamental difference between a
>> pending exception and an injected exception.
>> A pending exception means that no side-effects of the exception have
>> been applied yet. Including incrementing the RIP after the instruction
>> which cause exception. In our case for example, handle_wrmsr() calls
>> kvm_inject_gp() and returns without calling
>> kvm_skip_emulated_instruction() which increments the RIP.
> 
> Ok, I was almost sure this was going to be the case if the #GP wasn't
> lost (but couldn't convince myself 100%).
> 
>> Therefore, when we exit from L2 to L1 on vmx-preemption-timer, we can
>> safely clear exception.pending because when L1 will resume L2, the
>> exception will be raised again (the same WRMSR instruction will be run
>> again which will raise #GP again).
>> This is also why vmcs12_save_pending_event() only makes sure to save in
>> VMCS12 idt-vectoring-info the "injected" events and not the "pending"
>> events (interrupt.pending is misleading name and I would rename it in
>> upcoming patch to interrupt.injected. See explanation below).
> 
> Indeed.  And then kvm_event_needs_reinjection starts making sense.
> 
>> I can confirm this patch works because I have wrote a kvm-unit-test
>> which reproduce this issue. And after the fix the #GP is not lost and
>> raised to L2 directly correctly.
>> (I haven't posted the unit-test yet because it is very dependent on
>> correct vmx-preemption-timer timer config that varies between
>> environments).
> 
> Can you post it anyway?  Tests always help understanding the code.
> 
> Paolo
> 

Kindly pinging about this patch to understand if there is any work left to be done.
(kvm-unit-test reproducing issue was sent privately to Paolo a couple of weeks ago).

Thanks,
-Liran

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-01-09 10:08 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-19 16:25 [PATCH] KVM: nVMX: Fix bug of injecting L2 exception into L1 Liran Alon
2017-11-20 21:47 ` Paolo Bonzini
2017-11-20 22:46   ` Liran Alon
2017-11-20 22:58     ` Paolo Bonzini
2018-01-09 10:08 ` Paolo Bonzini
  -- strict thread matches above, loose matches on Subject: below --
2017-12-12  0:06 Liran Alon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox