diff mbox series

[RFC,v6,14/92] kvm: introspection: handle introspection commands before returning to guest

Message ID 20190809160047.8319-15-alazar@bitdefender.com (mailing list archive)
State New, archived
Headers show
Series VM introspection | expand

Commit Message

Adalbert Lazăr Aug. 9, 2019, 3:59 p.m. UTC 
From: Mihai Donțu <mdontu@bitdefender.com>

The introspection requests (KVM_REQ_INTROSPECTION) are checked by any
introspected vCPU in two places:

  * on its way to guest - vcpu_enter_guest()
  * when halted - kvm_vcpu_block()

In kvm_vcpu_block(), we check to see if there are any introspection
requests during the swait loop, handle them outside of swait loop and
start swait again.

Signed-off-by: Mihai Donțu <mdontu@bitdefender.com>
Co-developed-by: Mircea Cîrjaliu <mcirjaliu@bitdefender.com>
Signed-off-by: Mircea Cîrjaliu <mcirjaliu@bitdefender.com>
Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>
---
 arch/x86/kvm/x86.c   |  3 +++
 include/linux/kvmi.h |  2 ++
 virt/kvm/kvm_main.c  | 28 ++++++++++++++++++++++------
 3 files changed, 27 insertions(+), 6 deletions(-)

Comments

Paolo Bonzini Aug. 13, 2019, 8:26 a.m. UTC | #1 
On 09/08/19 17:59, Adalbert Lazăr wrote:
> +			prepare_to_swait_exclusive(&vcpu->wq, &wait,
> +						   TASK_INTERRUPTIBLE);
> +
> +			if (kvm_vcpu_check_block(vcpu) < 0)
> +				break;
> +
> +			waited = true;
> +			schedule();
> +
> +			if (kvm_check_request(KVM_REQ_INTROSPECTION, vcpu)) {
> +				do_kvmi_work = true;
> +				break;
> +			}
> +		}
>  
> -		waited = true;
> -		schedule();
> +		finish_swait(&vcpu->wq, &wait);
> +
> +		if (do_kvmi_work)
> +			kvmi_handle_requests(vcpu);
> +		else
> +			break;
>  	}

Is this needed?  Or can it just go back to KVM_RUN and handle
KVM_REQ_INTROSPECTION there (in which case it would be basically
premature optimization)?

Paolo
Adalbert Lazăr Aug. 13, 2019, 1:54 p.m. UTC | #2 
On Tue, 13 Aug 2019 10:26:29 +0200, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 09/08/19 17:59, Adalbert Lazăr wrote:
> > +			prepare_to_swait_exclusive(&vcpu->wq, &wait,
> > +						   TASK_INTERRUPTIBLE);
> > +
> > +			if (kvm_vcpu_check_block(vcpu) < 0)
> > +				break;
> > +
> > +			waited = true;
> > +			schedule();
> > +
> > +			if (kvm_check_request(KVM_REQ_INTROSPECTION, vcpu)) {
> > +				do_kvmi_work = true;
> > +				break;
> > +			}
> > +		}
> >  
> > -		waited = true;
> > -		schedule();
> > +		finish_swait(&vcpu->wq, &wait);
> > +
> > +		if (do_kvmi_work)
> > +			kvmi_handle_requests(vcpu);
> > +		else
> > +			break;
> >  	}
> 
> Is this needed?  Or can it just go back to KVM_RUN and handle
> KVM_REQ_INTROSPECTION there (in which case it would be basically
> premature optimization)?
> 

It might still be needed, unless we can get back to this function.

The original commit message for this change was this:

    kvm: do not abort kvm_vcpu_block() in order to handle KVMI requests
    
    Leaving kvm_vcpu_block() in order to handle a request such as 'pause',
    would cause the vCPU to enter the guest when resumed. Most of the
    time this does not appear to be an issue, but during early boot it
    can happen for a non-boot vCPU to start executing code from areas that
    first needed to be set up by vCPU #0.
    
    In a particular case, vCPU #1 executed code which resided in an area
    not covered by a memslot, which caused an EPT violation that got
    turned in mmu_set_spte() into a MMIO request that required emulation.
    Unfortunatelly, the emulator tripped, exited to userspace and the VM
    was aborted.
Paolo Bonzini Aug. 13, 2019, 2:45 p.m. UTC | #3 
On 13/08/19 15:54, Adalbert Lazăr wrote:
>     Leaving kvm_vcpu_block() in order to handle a request such as 'pause',
>     would cause the vCPU to enter the guest when resumed. Most of the
>     time this does not appear to be an issue, but during early boot it
>     can happen for a non-boot vCPU to start executing code from areas that
>     first needed to be set up by vCPU #0.
>     
>     In a particular case, vCPU #1 executed code which resided in an area
>     not covered by a memslot, which caused an EPT violation that got
>     turned in mmu_set_spte() into a MMIO request that required emulation.
>     Unfortunatelly, the emulator tripped, exited to userspace and the VM
>     was aborted.

Okay, this makes sense.  Maybe you want to handle KVM_REQ_INTROSPECTION
in vcpu_run rather than vcpu_enter_guest?

Paolo
Adalbert Lazăr Aug. 14, 2019, 9:39 a.m. UTC | #4 
On Tue, 13 Aug 2019 16:45:11 +0200, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 13/08/19 15:54, Adalbert Lazăr wrote:
> >     Leaving kvm_vcpu_block() in order to handle a request such as 'pause',
> >     would cause the vCPU to enter the guest when resumed. Most of the
> >     time this does not appear to be an issue, but during early boot it
> >     can happen for a non-boot vCPU to start executing code from areas that
> >     first needed to be set up by vCPU #0.
> >     
> >     In a particular case, vCPU #1 executed code which resided in an area
> >     not covered by a memslot, which caused an EPT violation that got
> >     turned in mmu_set_spte() into a MMIO request that required emulation.
> >     Unfortunatelly, the emulator tripped, exited to userspace and the VM
> >     was aborted.
> 
> Okay, this makes sense.  Maybe you want to handle KVM_REQ_INTROSPECTION
> in vcpu_run rather than vcpu_enter_guest?
> 
> Paolo

Right! We've missed that.
diff mbox series

Patch

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0163e1ad1aaa..adbdb1ceb618 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7742,6 +7742,9 @@  static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		 */
 		if (kvm_check_request(KVM_REQ_HV_STIMER, vcpu))
 			kvm_hv_process_stimers(vcpu);
+
+		if (kvm_check_request(KVM_REQ_INTROSPECTION, vcpu))
+			kvmi_handle_requests(vcpu);
 	}
 
 	if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
diff --git a/include/linux/kvmi.h b/include/linux/kvmi.h
index e8d25d7da751..ae5de1905b55 100644
--- a/include/linux/kvmi.h
+++ b/include/linux/kvmi.h
@@ -16,6 +16,7 @@  int kvmi_ioctl_event(struct kvm *kvm, void __user *argp);
 int kvmi_ioctl_unhook(struct kvm *kvm, bool force_reset);
 int kvmi_vcpu_init(struct kvm_vcpu *vcpu);
 void kvmi_vcpu_uninit(struct kvm_vcpu *vcpu);
+void kvmi_handle_requests(struct kvm_vcpu *vcpu);
 
 #else
 
@@ -25,6 +26,7 @@  static inline void kvmi_create_vm(struct kvm *kvm) { }
 static inline void kvmi_destroy_vm(struct kvm *kvm) { }
 static inline int kvmi_vcpu_init(struct kvm_vcpu *vcpu) { return 0; }
 static inline void kvmi_vcpu_uninit(struct kvm_vcpu *vcpu) { }
+static inline void kvmi_handle_requests(struct kvm_vcpu *vcpu) { }
 
 #endif /* CONFIG_KVM_INTROSPECTION */
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 94f15f393e37..2e11069b9565 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2282,16 +2282,32 @@  void kvm_vcpu_block(struct kvm_vcpu *vcpu)
 	kvm_arch_vcpu_blocking(vcpu);
 
 	for (;;) {
-		prepare_to_swait_exclusive(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);
+		bool do_kvmi_work = false;
 
-		if (kvm_vcpu_check_block(vcpu) < 0)
-			break;
+		for (;;) {
+			prepare_to_swait_exclusive(&vcpu->wq, &wait,
+						   TASK_INTERRUPTIBLE);
+
+			if (kvm_vcpu_check_block(vcpu) < 0)
+				break;
+
+			waited = true;
+			schedule();
+
+			if (kvm_check_request(KVM_REQ_INTROSPECTION, vcpu)) {
+				do_kvmi_work = true;
+				break;
+			}
+		}
 
-		waited = true;
-		schedule();
+		finish_swait(&vcpu->wq, &wait);
+
+		if (do_kvmi_work)
+			kvmi_handle_requests(vcpu);
+		else
+			break;
 	}
 
-	finish_swait(&vcpu->wq, &wait);
 	cur = ktime_get();
 
 	kvm_arch_vcpu_unblocking(vcpu);