Re: [PATCH] KVM: x86: Avoid NULL dereference in kvm_apic_accept_pic_intr()

[Marcelo Tosatti <mtosatti@redhat.com>]

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 <michael@ellerman.id.au>
> ---
>  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