diff mbox series

[v2,2/2] KVM: x86: Fix access to vcpu->arch.apic when the irqchip is not in kernel

Message ID 20220706145957.32156-2-juew@google.com (mailing list archive)
State New, archived
Headers show
Series [v2,1/2] KVM: x86: Initialize nr_lvt_entries to a proper default value | expand

Commit Message

Jue Wang July 6, 2022, 2:59 p.m. UTC 
Fix an access to vcpu->arch.apic when KVM_X86_SETUP_MCE is called
without KVM_CREATE_IRQCHIP called or KVM_CAP_SPLIT_IRQCHIP is
enabled.

Reported-by: https://syzkaller.appspot.com/bug?id=10b9b238e087a6c9bef2cc48bee2375f58fabbfc

Fixes: 4b903561ec49 ("KVM: x86: Add Corrected Machine Check Interrupt (CMCI) emulation to lapic.")
Signed-off-by: Jue Wang <juew@google.com>
---
 arch/x86/kvm/x86.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Siddh Raman Pant July 6, 2022, 4:08 p.m. UTC | #1 
On Wed, 06 Jul 2022 20:29:57 +0530  Jue Wang <juew@google.com> wrote
> Fix an access to vcpu->arch.apic when KVM_X86_SETUP_MCE is called
> without KVM_CREATE_IRQCHIP called or KVM_CAP_SPLIT_IRQCHIP is
> enabled.
> 
> Reported-by: https://syzkaller.appspot.com/bug?id=10b9b238e087a6c9bef2cc48bee2375f58fabbfc
> 
> Fixes: 4b903561ec49 ("KVM: x86: Add Corrected Machine Check Interrupt (CMCI) emulation to lapic.")
> Signed-off-by: Jue Wang <juew@google.com>
> ---
>  arch/x86/kvm/x86.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 

The syzkaller dashboard says to use the following line:
Reported-by: syzbot+8cdad6430c24f396f158@syzkaller.appspotmail.com


Tested-by: Siddh Raman Pant <code@siddh.me>

Thanks,
Siddh

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4322a1365f74..5913f90ec3f2 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4820,8 +4820,9 @@ static int kvm_vcpu_ioctl_x86_setup_mce(struct kvm_vcpu *vcpu,
>          if (mcg_cap & MCG_CMCI_P)
>              vcpu->arch.mci_ctl2_banks[bank] = 0;
>      }
> -    vcpu->arch.apic->nr_lvt_entries =
> -        KVM_APIC_MAX_NR_LVT_ENTRIES - !(mcg_cap & MCG_CMCI_P);
> +    if (lapic_in_kernel(vcpu))
> +        vcpu->arch.apic->nr_lvt_entries =
> +            KVM_APIC_MAX_NR_LVT_ENTRIES - !(mcg_cap & MCG_CMCI_P);
>  
>      static_call(kvm_x86_setup_mce)(vcpu);
>  out:
> -- 
> 2.37.0.rc0.161.g10f37bed90-goog
> 
>
Sean Christopherson July 8, 2022, 10:59 p.m. UTC | #2 
On Wed, Jul 06, 2022, Jue Wang wrote:
> Fix an access to vcpu->arch.apic when KVM_X86_SETUP_MCE is called
> without KVM_CREATE_IRQCHIP called or KVM_CAP_SPLIT_IRQCHIP is
> enabled.
> 
> Reported-by: https://syzkaller.appspot.com/bug?id=10b9b238e087a6c9bef2cc48bee2375f58fabbfc
> 
> Fixes: 4b903561ec49 ("KVM: x86: Add Corrected Machine Check Interrupt (CMCI) emulation to lapic.")
> Signed-off-by: Jue Wang <juew@google.com>
> ---
>  arch/x86/kvm/x86.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4322a1365f74..5913f90ec3f2 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4820,8 +4820,9 @@ static int kvm_vcpu_ioctl_x86_setup_mce(struct kvm_vcpu *vcpu,
>  		if (mcg_cap & MCG_CMCI_P)
>  			vcpu->arch.mci_ctl2_banks[bank] = 0;
>  	}
> -	vcpu->arch.apic->nr_lvt_entries =
> -		KVM_APIC_MAX_NR_LVT_ENTRIES - !(mcg_cap & MCG_CMCI_P);
> +	if (lapic_in_kernel(vcpu))
> +		vcpu->arch.apic->nr_lvt_entries =
> +			KVM_APIC_MAX_NR_LVT_ENTRIES - !(mcg_cap & MCG_CMCI_P);

This is incomplete.  If there's a "new" LVT entry, then it needs to be initialized
(masked), and the APIC version needs to be updated to reflect the up-to-date number
of LVT entries.

This is what I came up with, again compile tested only, will formally post next
week.

From: Sean Christopherson <seanjc@google.com>
Date: Fri, 8 Jul 2022 15:48:10 -0700
Subject: [PATCH] KVM: x86: Fix handling of APIC LVT updates when userspace
 changes MCG_CAP

Add a helper to update KVM's in-kernel local APIC in response to MCG_CAP
being changed by userspace to fix multiple bugs.  First and foremost,
KVM needs to check that there's an in-kernel APIC prior to dereferencing
vcpu->arch.apic.  Beyond that, any "new" LVT entries need to be masked,
and the APIC version register needs to be updated as it reports out the
number of LVT entries.

Fixes: 4b903561ec49 ("KVM: x86: Add Corrected Machine Check Interrupt (CMCI) emulation to lapic.")
Reported-by: syzbot+8cdad6430c24f396f158@syzkaller.appspotmail.com
Cc: Siddh Raman Pant <code@siddh.me>
Cc: Jue Wang <juew@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/lapic.c | 19 +++++++++++++++++++
 arch/x86/kvm/lapic.h |  1 +
 arch/x86/kvm/x86.c   |  4 ++--
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 1540d01ecb67..50354c7a2dc1 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -433,6 +433,25 @@ void kvm_apic_set_version(struct kvm_vcpu *vcpu)
 	kvm_lapic_set_reg(apic, APIC_LVR, v);
 }

+void kvm_apic_after_set_mcg_cap(struct kvm_vcpu *vcpu)
+{
+	int nr_lvt_entries = kvm_apic_calc_nr_lvt_entries(vcpu);
+	struct kvm_lapic *apic = vcpu->arch.apic;
+	int i;
+
+	if (!lapic_in_kernel(vcpu) || nr_lvt_entries == apic->nr_lvt_entries)
+		return;
+
+	/* Initialize/mask any "new" LVT entries. */
+	for (i = apic->nr_lvt_entries; i < nr_lvt_entries; i++)
+		kvm_lapic_set_reg(apic, APIC_LVTx(i), APIC_LVT_MASKED);
+
+	apic->nr_lvt_entries = nr_lvt_entries;
+
+	/* The number of LVT entries is reflected in the version register. */
+	kvm_apic_set_version(vcpu);
+}
+
 static const unsigned int apic_lvt_mask[KVM_APIC_MAX_NR_LVT_ENTRIES] = {
 	[LVT_TIMER] = LVT_MASK,      /* timer mode mask added at runtime */
 	[LVT_THERMAL_MONITOR] = LVT_MASK | APIC_MODE_MASK,
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 762bf6163798..117a46df5cc1 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -99,6 +99,7 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value);
 u64 kvm_lapic_get_base(struct kvm_vcpu *vcpu);
 void kvm_recalculate_apic_map(struct kvm *kvm);
 void kvm_apic_set_version(struct kvm_vcpu *vcpu);
+void kvm_apic_after_set_mcg_cap(struct kvm_vcpu *vcpu);
 bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
 			   int shorthand, unsigned int dest, int dest_mode);
 int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fb37d11dec2d..801c3cfd3db5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4893,8 +4893,8 @@ static int kvm_vcpu_ioctl_x86_setup_mce(struct kvm_vcpu *vcpu,
 		if (mcg_cap & MCG_CMCI_P)
 			vcpu->arch.mci_ctl2_banks[bank] = 0;
 	}
-	vcpu->arch.apic->nr_lvt_entries =
-		KVM_APIC_MAX_NR_LVT_ENTRIES - !(mcg_cap & MCG_CMCI_P);
+
+	kvm_apic_after_set_mcg_cap(vcpu);

 	static_call(kvm_x86_setup_mce)(vcpu);
 out:

base-commit: 03d84f96890662681feee129cf92491f49247d28
--
Jue Wang July 8, 2022, 11:03 p.m. UTC | #3 
Thanks Sean.

+cc another email to stay in the loop.

On Fri, Jul 8, 2022 at 3:59 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Jul 06, 2022, Jue Wang wrote:
> > Fix an access to vcpu->arch.apic when KVM_X86_SETUP_MCE is called
> > without KVM_CREATE_IRQCHIP called or KVM_CAP_SPLIT_IRQCHIP is
> > enabled.
> >
> > Reported-by: https://syzkaller.appspot.com/bug?id=10b9b238e087a6c9bef2cc48bee2375f58fabbfc
> >
> > Fixes: 4b903561ec49 ("KVM: x86: Add Corrected Machine Check Interrupt (CMCI) emulation to lapic.")
> > Signed-off-by: Jue Wang <juew@google.com>
> > ---
> >  arch/x86/kvm/x86.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 4322a1365f74..5913f90ec3f2 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -4820,8 +4820,9 @@ static int kvm_vcpu_ioctl_x86_setup_mce(struct kvm_vcpu *vcpu,
> >               if (mcg_cap & MCG_CMCI_P)
> >                       vcpu->arch.mci_ctl2_banks[bank] = 0;
> >       }
> > -     vcpu->arch.apic->nr_lvt_entries =
> > -             KVM_APIC_MAX_NR_LVT_ENTRIES - !(mcg_cap & MCG_CMCI_P);
> > +     if (lapic_in_kernel(vcpu))
> > +             vcpu->arch.apic->nr_lvt_entries =
> > +                     KVM_APIC_MAX_NR_LVT_ENTRIES - !(mcg_cap & MCG_CMCI_P);
>
> This is incomplete.  If there's a "new" LVT entry, then it needs to be initialized
> (masked), and the APIC version needs to be updated to reflect the up-to-date number
> of LVT entries.
>
> This is what I came up with, again compile tested only, will formally post next
> week.
>
> From: Sean Christopherson <seanjc@google.com>
> Date: Fri, 8 Jul 2022 15:48:10 -0700
> Subject: [PATCH] KVM: x86: Fix handling of APIC LVT updates when userspace
>  changes MCG_CAP
>
> Add a helper to update KVM's in-kernel local APIC in response to MCG_CAP
> being changed by userspace to fix multiple bugs.  First and foremost,
> KVM needs to check that there's an in-kernel APIC prior to dereferencing
> vcpu->arch.apic.  Beyond that, any "new" LVT entries need to be masked,
> and the APIC version register needs to be updated as it reports out the
> number of LVT entries.
>
> Fixes: 4b903561ec49 ("KVM: x86: Add Corrected Machine Check Interrupt (CMCI) emulation to lapic.")
> Reported-by: syzbot+8cdad6430c24f396f158@syzkaller.appspotmail.com
> Cc: Siddh Raman Pant <code@siddh.me>
> Cc: Jue Wang <juew@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/lapic.c | 19 +++++++++++++++++++
>  arch/x86/kvm/lapic.h |  1 +
>  arch/x86/kvm/x86.c   |  4 ++--
>  3 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 1540d01ecb67..50354c7a2dc1 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -433,6 +433,25 @@ void kvm_apic_set_version(struct kvm_vcpu *vcpu)
>         kvm_lapic_set_reg(apic, APIC_LVR, v);
>  }
>
> +void kvm_apic_after_set_mcg_cap(struct kvm_vcpu *vcpu)
> +{
> +       int nr_lvt_entries = kvm_apic_calc_nr_lvt_entries(vcpu);
> +       struct kvm_lapic *apic = vcpu->arch.apic;
> +       int i;
> +
> +       if (!lapic_in_kernel(vcpu) || nr_lvt_entries == apic->nr_lvt_entries)
> +               return;
> +
> +       /* Initialize/mask any "new" LVT entries. */
> +       for (i = apic->nr_lvt_entries; i < nr_lvt_entries; i++)
> +               kvm_lapic_set_reg(apic, APIC_LVTx(i), APIC_LVT_MASKED);
> +
> +       apic->nr_lvt_entries = nr_lvt_entries;
> +
> +       /* The number of LVT entries is reflected in the version register. */
> +       kvm_apic_set_version(vcpu);
> +}
> +
>  static const unsigned int apic_lvt_mask[KVM_APIC_MAX_NR_LVT_ENTRIES] = {
>         [LVT_TIMER] = LVT_MASK,      /* timer mode mask added at runtime */
>         [LVT_THERMAL_MONITOR] = LVT_MASK | APIC_MODE_MASK,
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 762bf6163798..117a46df5cc1 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -99,6 +99,7 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value);
>  u64 kvm_lapic_get_base(struct kvm_vcpu *vcpu);
>  void kvm_recalculate_apic_map(struct kvm *kvm);
>  void kvm_apic_set_version(struct kvm_vcpu *vcpu);
> +void kvm_apic_after_set_mcg_cap(struct kvm_vcpu *vcpu);
>  bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>                            int shorthand, unsigned int dest, int dest_mode);
>  int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fb37d11dec2d..801c3cfd3db5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4893,8 +4893,8 @@ static int kvm_vcpu_ioctl_x86_setup_mce(struct kvm_vcpu *vcpu,
>                 if (mcg_cap & MCG_CMCI_P)
>                         vcpu->arch.mci_ctl2_banks[bank] = 0;
>         }
> -       vcpu->arch.apic->nr_lvt_entries =
> -               KVM_APIC_MAX_NR_LVT_ENTRIES - !(mcg_cap & MCG_CMCI_P);
> +
> +       kvm_apic_after_set_mcg_cap(vcpu);
>
>         static_call(kvm_x86_setup_mce)(vcpu);
>  out:
>
> base-commit: 03d84f96890662681feee129cf92491f49247d28
> --
>
diff mbox series

Patch

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4322a1365f74..5913f90ec3f2 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4820,8 +4820,9 @@  static int kvm_vcpu_ioctl_x86_setup_mce(struct kvm_vcpu *vcpu,
 		if (mcg_cap & MCG_CMCI_P)
 			vcpu->arch.mci_ctl2_banks[bank] = 0;
 	}
-	vcpu->arch.apic->nr_lvt_entries =
-		KVM_APIC_MAX_NR_LVT_ENTRIES - !(mcg_cap & MCG_CMCI_P);
+	if (lapic_in_kernel(vcpu))
+		vcpu->arch.apic->nr_lvt_entries =
+			KVM_APIC_MAX_NR_LVT_ENTRIES - !(mcg_cap & MCG_CMCI_P);
 
 	static_call(kvm_x86_setup_mce)(vcpu);
 out: