Thanks Sean. +cc another email to stay in the loop. On Fri, Jul 8, 2022 at 3:59 PM Sean Christopherson <seanjc@xxxxxxxxxx> 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@xxxxxxxxxx> > > --- > > 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@xxxxxxxxxx> > 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@xxxxxxxxxxxxxxxxxxxxxxxxx > Cc: Siddh Raman Pant <code@xxxxxxxx> > Cc: Jue Wang <juew@xxxxxxxxxx> > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx> > --- > 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 > -- >
