From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH] KVM: x86: Avoid NULL dereference in kvm_apic_accept_pic_intr() Date: Tue, 7 Feb 2012 17:38:39 -0200 Message-ID: <20120207193839.GA20281@amt.cnet> References: <1328596327-18662-1-git-send-email-michael@ellerman.id.au> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, avi@redhat.com To: Michael Ellerman Return-path: Received: from mx1.redhat.com ([209.132.183.28]:55852 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752447Ab2BGTki (ORCPT ); Tue, 7 Feb 2012 14:40:38 -0500 Content-Disposition: inline In-Reply-To: <1328596327-18662-1-git-send-email-michael@ellerman.id.au> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, Feb 07, 2012 at 05:32:07PM +1100, Michael Ellerman wrote: > A test case which does the following: > > ioctl(vmfd, KVM_CREATE_VCPU, 0); > ioctl(vmfd, KVM_CREATE_IRQCHIP); > ioctl(cpufd, KVM_RUN); > > Can oops in kvm_apic_accept_pic_intr() because vcpu->arch.apic == NULL. > > Because irqchip_in_kernel() is false when we create the vcpu we leave > vcpu->arch.apic uninitialised (in kvm_arch_vcpu_init()). Then when we run, > irqchip_in_kernel() is true, but we didn't do the correct initialisation. > > The root of the problem seems to be that there is an assumption that > KVM_CREATE_IRQCHIP will be called before any VCPUs are created. The > documentation says "sets up future vcpus to have a local APIC". > > So the simplest fix seems to be to enforce that ordering in the code. Ugh. With your patch below there is still the window for a race: kvm_arch_vcpu_init can create a vcpu without vcpu->arch.apic, block on mutex_lock(kvm->lock). Meanwhile a separate thread is on KVM_CREATE_IRQCHIP holding kvm->lock, finds online_vcpus == 0 and proceeds. Then kvm_arch_vcpu_init finishes. Moving mutex_lock(kvm->lock) to the beginning of kvm_vm_ioctl_create_vcpu should fix it? > > Signed-off-by: Michael Ellerman > --- > arch/x86/kvm/x86.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 14d6cad..27dd380 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -3110,6 +3110,9 @@ long kvm_arch_vm_ioctl(struct file *filp, > r = -EEXIST; > if (kvm->arch.vpic) > goto create_irqchip_unlock; > + r = -EINVAL; > + if (atomic_read(&kvm->online_vcpus)) > + goto create_irqchip_unlock; > r = -ENOMEM; > vpic = kvm_create_pic(kvm); > if (vpic) { > -- > 1.7.5.4 > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html