diff mbox series

[05/12] KVM: x86: Add a return code to kvm_apic_accept_events

Message ID 20210520230339.267445-6-jmattson@google.com (mailing list archive)
State New, archived
Headers show
Series KVM: nVMX: Fix vmcs02 PID use-after-free issue | expand

Commit Message

Jim Mattson May 20, 2021, 11:03 p.m. UTC 
No functional change intended. At present, the only negative value
returned by kvm_check_nested_events is -EBUSY.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/lapic.c | 11 ++++++-----
 arch/x86/kvm/lapic.h |  2 +-
 arch/x86/kvm/x86.c   | 22 ++++++++++++++++++----
 3 files changed, 25 insertions(+), 10 deletions(-)

Comments

Reiji Watanabe May 25, 2021, 7:24 p.m. UTC | #1 
> @@ -9880,11 +9888,16 @@ int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
>  int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
>                                     struct kvm_mp_state *mp_state)
>  {
> +       int r = 0;
> +
>         vcpu_load(vcpu);
>         if (kvm_mpx_supported())
>                 kvm_load_guest_fpu(vcpu);
>
> -       kvm_apic_accept_events(vcpu);
> +       r = kvm_apic_accept_events(vcpu);
> +       if (r < 0)
> +               goto out;
> +
>         if ((vcpu->arch.mp_state == KVM_MP_STATE_HALTED ||
>              vcpu->arch.mp_state == KVM_MP_STATE_AP_RESET_HOLD) &&
>             vcpu->arch.pv.pv_unhalted)
> @@ -9892,6 +9905,7 @@ int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
>         else
>                 mp_state->mp_state = vcpu->arch.mp_state;
>
> +out:
>         if (kvm_mpx_supported())
>                 kvm_put_guest_fpu(vcpu);
>         vcpu_put(vcpu);

With the change, if the return value from kvm_apic_accept_events()
is < 0, kvm_arch_vcpu_ioctl_get_mpstate(), which is called from
KVM_GET_MP_STATE ioctl, doesn't set mp_state returning 0 (success).
It leads KVM_GET_MP_STATE ioctl to return an undefined mp_state for
the success case.

Thanks,
Reiji
Jim Mattson May 25, 2021, 8:35 p.m. UTC | #2 
On Tue, May 25, 2021 at 12:24 PM Reiji Watanabe <reijiw@google.com> wrote:
>
> > @@ -9880,11 +9888,16 @@ int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
> >  int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
> >                                     struct kvm_mp_state *mp_state)
> >  {
> > +       int r = 0;
> > +
> >         vcpu_load(vcpu);
> >         if (kvm_mpx_supported())
> >                 kvm_load_guest_fpu(vcpu);
> >
> > -       kvm_apic_accept_events(vcpu);
> > +       r = kvm_apic_accept_events(vcpu);
> > +       if (r < 0)
> > +               goto out;
> > +
> >         if ((vcpu->arch.mp_state == KVM_MP_STATE_HALTED ||
> >              vcpu->arch.mp_state == KVM_MP_STATE_AP_RESET_HOLD) &&
> >             vcpu->arch.pv.pv_unhalted)
> > @@ -9892,6 +9905,7 @@ int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
> >         else
> >                 mp_state->mp_state = vcpu->arch.mp_state;
> >
> > +out:
> >         if (kvm_mpx_supported())
> >                 kvm_put_guest_fpu(vcpu);
> >         vcpu_put(vcpu);
>
> With the change, if the return value from kvm_apic_accept_events()
> is < 0, kvm_arch_vcpu_ioctl_get_mpstate(), which is called from
> KVM_GET_MP_STATE ioctl, doesn't set mp_state returning 0 (success).
> It leads KVM_GET_MP_STATE ioctl to return an undefined mp_state for
> the success case.

Yikes! I think I intended to return 'r' when it is less than 0 (e.g.
the -ENXIO I introduce later in the series). However, I'm not quite
sure what to do with values of r > 0. I'll look into it and send out
v2.
diff mbox series

Patch

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index c0ebef560bd1..f747864ff323 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2856,7 +2856,7 @@  int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data, unsigned long len)
 	return kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, addr, new_len);
 }
 
-void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
+int kvm_apic_accept_events(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 	u8 sipi_vector;
@@ -2864,7 +2864,7 @@  void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
 	unsigned long pe;
 
 	if (!lapic_in_kernel(vcpu))
-		return;
+		return 0;
 
 	/*
 	 * Read pending events before calling the check_events
@@ -2872,12 +2872,12 @@  void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
 	 */
 	pe = smp_load_acquire(&apic->pending_events);
 	if (!pe)
-		return;
+		return 0;
 
 	if (is_guest_mode(vcpu)) {
 		r = kvm_check_nested_events(vcpu);
 		if (r < 0)
-			return;
+			return r == -EBUSY ? 0 : r;
 		/*
 		 * If an event has happened and caused a vmexit,
 		 * we know INITs are latched and therefore
@@ -2898,7 +2898,7 @@  void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
 		WARN_ON_ONCE(vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED);
 		if (test_bit(KVM_APIC_SIPI, &pe))
 			clear_bit(KVM_APIC_SIPI, &apic->pending_events);
-		return;
+		return 0;
 	}
 
 	if (test_bit(KVM_APIC_INIT, &pe)) {
@@ -2919,6 +2919,7 @@  void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
 			vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
 		}
 	}
+	return 0;
 }
 
 void kvm_lapic_exit(void)
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 997c45a5963a..d7c25d0c1354 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -76,7 +76,7 @@  void kvm_free_lapic(struct kvm_vcpu *vcpu);
 int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu);
 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu);
 int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu);
-void kvm_apic_accept_events(struct kvm_vcpu *vcpu);
+int kvm_apic_accept_events(struct kvm_vcpu *vcpu);
 void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event);
 u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu);
 void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index eb35536f8d06..9258fee4f272 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9242,7 +9242,11 @@  static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win ||
 	    kvm_xen_has_interrupt(vcpu)) {
 		++vcpu->stat.req_event;
-		kvm_apic_accept_events(vcpu);
+		r = kvm_apic_accept_events(vcpu);
+		if (r < 0) {
+			r = 0;
+			goto out;
+		}
 		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
 			r = 1;
 			goto out;
@@ -9454,7 +9458,8 @@  static inline int vcpu_block(struct kvm *kvm, struct kvm_vcpu *vcpu)
 			return 1;
 	}
 
-	kvm_apic_accept_events(vcpu);
+	if (kvm_apic_accept_events(vcpu) < 0)
+		return 0;
 	switch(vcpu->arch.mp_state) {
 	case KVM_MP_STATE_HALTED:
 	case KVM_MP_STATE_AP_RESET_HOLD:
@@ -9678,7 +9683,10 @@  int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 			goto out;
 		}
 		kvm_vcpu_block(vcpu);
-		kvm_apic_accept_events(vcpu);
+		if (kvm_apic_accept_events(vcpu) < 0) {
+			r = 0;
+			goto out;
+		}
 		kvm_clear_request(KVM_REQ_UNHALT, vcpu);
 		r = -EAGAIN;
 		if (signal_pending(current)) {
@@ -9880,11 +9888,16 @@  int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 				    struct kvm_mp_state *mp_state)
 {
+	int r = 0;
+
 	vcpu_load(vcpu);
 	if (kvm_mpx_supported())
 		kvm_load_guest_fpu(vcpu);
 
-	kvm_apic_accept_events(vcpu);
+	r = kvm_apic_accept_events(vcpu);
+	if (r < 0)
+		goto out;
+
 	if ((vcpu->arch.mp_state == KVM_MP_STATE_HALTED ||
 	     vcpu->arch.mp_state == KVM_MP_STATE_AP_RESET_HOLD) &&
 	    vcpu->arch.pv.pv_unhalted)
@@ -9892,6 +9905,7 @@  int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 	else
 		mp_state->mp_state = vcpu->arch.mp_state;
 
+out:
 	if (kvm_mpx_supported())
 		kvm_put_guest_fpu(vcpu);
 	vcpu_put(vcpu);