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

From: Jue Wang <juew@google.com>
To: Siddh Raman Pant <code@siddh.me>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	Sean Christopherson <seanjc@google.com>,
	Jim Mattson <jmattson@google.com>,
	Xiaoyao Li <xiaoyao.li@intel.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Joerg Roedel <joro@8bytes.org>,
	David Matlack <dmatlack@google.com>,
	Tony Luck <tony.luck@intel.com>, kvm <kvm@vger.kernel.org>,
	Jiaqi Yan <jiaqiyan@google.com>,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] KVM: x86: Fix access to vcpu->arch.apic when the irqchip is not in kernel
Date: Wed, 6 Jul 2022 08:07:29 -0700	[thread overview]
Message-ID: <CAPcxDJ51EQwTZzNOzckRGXgE9s6X+rpDWFxvb8JZpAQQVjm1iQ@mail.gmail.com> (raw)
In-Reply-To: <181c484aa33.6db8a9c7835812.4939150843849434525@siddh.me>

Hi Siddh,

Thanks for the note.

I've sent out an updated v2 patch:
https://lore.kernel.org/kvm/20220706145957.32156-2-juew@google.com/T/#u

Thanks,
-Jue

On Sun, Jul 3, 2022 at 7:44 AM Siddh Raman Pant <code@siddh.me> wrote:
>
> On Fri, 01 Jul 2022 22:20:45 +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.
> >
> > 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..d81020dd0fea 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 (vcpu->arch.apic)
> > +        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
> >
> >
>
> Hello Jue,
>
> There is a syzkaller bug regarding null ptr dereference which is caused by
> vcpu->arch.apic being NULL, first reported on 27th June. You might want to
> add it's reported-by line so that it can be marked as fixed.
>
> Link: https://syzkaller.appspot.com/bug?id=10b9b238e087a6c9bef2cc48bee2375f58fabbfc
>
> I was looking at this bug too and fixed it (i.e. reproducer won't crash)
> using lapic_in_kernel(vcpu) as a condition instead of null ptr check on
> vcpu->arch.apic, as it makes more sense to the code reader (the lapic is
> not there since during kvm_arch_vcpu_create(), it isn't created due to
> irqchip_in_kernel() check being false).
>
> May I suggest that lapic_in_kernel(vcpu) be used instead of the null ptr
> check?
>
> Thanks,
> Siddh