linux-kernel - Re: [PATCH] KVM: x86: check for pic and ioapic presence before use

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2119448151.1732997.1480005286539.JavaMail.zimbra@redhat.com>
Date:   Thu, 24 Nov 2016 11:34:46 -0500 (EST)
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Radim Krčmář <rkrcmar@...hat.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        stable@...r.kernel.org, Dmitry Vyukov <dvyukov@...gle.com>,
        Steve Rutherford <srutherford@...gle.com>
Subject: Re: [PATCH] KVM: x86: check for pic and ioapic presence before use


> Oops, I wrote the race with wrong IOCTL -- it should be KVM_IRQ_LINE.
> 
>  1) set KVM_CAP_SPLIT_IRQCHIP (unlocks KVM_IRQ_LINE)
>  a) call KVM_CREATE_IRQCHIP (creates routes while !kvm->arch.vpic)
>  b) concurrently call KVM_IRQ_LINE for PIO routes (dereferences NULL)
> 
> The problem is that we use pic_in_kernel() as irqchip_in_kernel(), so it
> cannot be set before we set up routes, but we then cannot reject routes
> when pic is not in use.  The best effort is to do this for pic routes in
> kvm_set_routing_entry():
> 
>  // initialization is the only place where pic_in_kernel() !=
>  ioapic_in_kernel()
>  if (!pic_in_kernel(kvm) && !ioapic_in_kernel(kvm))
>  	goto out;
> 
> and similar for ioapic routes:
> 
>  if (!ioapic_in_kernel(kvm))
>  	goto out;
> 
> I think it would work if we forbade KVM_CREATE_IRQCHIP after
> KVM_CAP_SPLIT_IRQCHIP (which we want to do anyway).

Yeah, definitely.

> And adding a new
> variable for irqchip_in_kernel() would allow us to make the pic
> condition reasonabled.

Or change kvm->arch.irqchip_split to an enum.

> I'll do something like that for 4.10, but the current patch is better
> suited for stable.
> 
> Would fixing the comment be enough?

Yes, fine!

> Do you want the following hunk already in 4.9?
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 6f9c9ad13f88..dbed51045c37 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3901,7 +3901,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  
>  		mutex_lock(&kvm->lock);
>  		r = -EEXIST;
> -		if (kvm->arch.vpic)
> +		if (irqchip_in_kernel(kvm))
>  			goto create_irqchip_unlock;
>  		r = -EINVAL;
>  		if (kvm->created_vcpus)

No, it's unnecessary.

Paolo