Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 03 Jul 2022 20:13:57 +0530
From:   Siddh Raman Pant <code@siddh.me>
To:     "Jue Wang" <juew@google.com>
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>
Message-ID: <181c484aa33.6db8a9c7835812.4939150843849434525@siddh.me>
In-Reply-To: <20220701165045.4074471-2-juew@google.com>
References: <20220701165045.4074471-1-juew@google.com> <20220701165045.4074471-2-juew@google.com>
Subject: Re: [PATCH 2/2] KVM: x86: Fix access to vcpu->arch.apic when the
 irqchip is not in kernel
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Importance: Medium
User-Agent: Zoho Mail
Precedence: bulk

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