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

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

[Michael Ellerman]

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.

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

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

[Marcelo Tosatti]

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

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

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1753 bytes --]

On Tue, 2012-02-07 at 17:38 -0200, Marcelo Tosatti wrote:
> 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.

Yeah bugger. I missed that most of the vcpu create is done without the
mutex held.

> Moving mutex_lock(kvm->lock) to the beginning of
> kvm_vm_ioctl_create_vcpu should fix it?

Yeah I think so. The slight problem is that arch code in the formerly
unlocked vcpu create path might take the mutex, powerpc does, but AFAICS
that is the only arch that does. So as long as we remove that it should
work.

I'm travelling today and tomorrow but I'll get you a new patch as soon
as I get a chance.

cheers

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2509 bytes --]

On Wed, 2012-02-08 at 21:41 +1100, Michael Ellerman wrote:
> On Tue, 2012-02-07 at 17:38 -0200, Marcelo Tosatti wrote:
> > 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.
> 
> Yeah bugger. I missed that most of the vcpu create is done without the
> mutex held.
> 
> > Moving mutex_lock(kvm->lock) to the beginning of
> > kvm_vm_ioctl_create_vcpu should fix it?

Hmm, maybe not.

How are the locks meant to nest?

If we move the mutex up, we will be calling kvm_arch_vcpu_setup() with
the kvm->lock held, which calls vcpu_load() which takes vcpu->mutex.

So we would be taking the kvm->lock (A) then vcpu->mutex (B).

And I think the following path takes vcpu->mutex (B) then kvm->lock (A).

kvm_vcpu_ioctl
 -> vcpu_load
    -> mutex_lock(&vcpu->mutex);
 -> kvm_arch_vcpu_ioctl()
    KVM_GET_MSRS:
    -> msr_io(vcpu, argp, kvm_get_msr, 1)
      -> __msr_io(..)
        -> srcu_read_lock(&vcpu->kvm->srcu);
        -> kvm_get_msr()
          -> kvm_x86_ops->get_msr()
            -> vmx_get_msr()
               default:
               -> kvm_get_msr_common()
                  case HV_X64_MSR_GUEST_OS_ID ... HV_X64_MSR_SINT15:
                  -> mutex_lock(&vcpu->kvm->lock);


But trawling through that code path was a bit of a mess, so maybe I
missed something somewhere.

cheers




[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Avi Kivity]

On 02/08/2012 02:13 PM, Michael Ellerman wrote:
> On Wed, 2012-02-08 at 21:41 +1100, Michael Ellerman wrote:
> > On Tue, 2012-02-07 at 17:38 -0200, Marcelo Tosatti wrote:
> > > 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.
> > 
> > Yeah bugger. I missed that most of the vcpu create is done without the
> > mutex held.
> > 
> > > Moving mutex_lock(kvm->lock) to the beginning of
> > > kvm_vm_ioctl_create_vcpu should fix it?
>
> Hmm, maybe not.
>
> How are the locks meant to nest?
>
> If we move the mutex up, we will be calling kvm_arch_vcpu_setup() with
> the kvm->lock held, which calls vcpu_load() which takes vcpu->mutex.
>
> So we would be taking the kvm->lock (A) then vcpu->mutex (B).
>
> And I think the following path takes vcpu->mutex (B) then kvm->lock (A).
>
> kvm_vcpu_ioctl
>  -> vcpu_load
>     -> mutex_lock(&vcpu->mutex);
>  -> kvm_arch_vcpu_ioctl()
>     KVM_GET_MSRS:
>     -> msr_io(vcpu, argp, kvm_get_msr, 1)
>       -> __msr_io(..)
>         -> srcu_read_lock(&vcpu->kvm->srcu);
>         -> kvm_get_msr()
>           -> kvm_x86_ops->get_msr()
>             -> vmx_get_msr()
>                default:
>                -> kvm_get_msr_common()
>                   case HV_X64_MSR_GUEST_OS_ID ... HV_X64_MSR_SINT15:
>                   -> mutex_lock(&vcpu->kvm->lock);
>
>
> But trawling through that code path was a bit of a mess, so maybe I
> missed something somewhere.

I think you are correct (the path is benign, since the vcpu being
created is not going to run and read that MSR, but let's not get lockdep
angry at us).

However kvm_arch_vcpu_init(), which creates the lapic, _is_ called
without either the vcpu->mutex or kvm->lock held.  Patch coming up.

-- 
error compiling committee.c: too many arguments to function

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

[Avi Kivity]

On 03/04/2012 11:51 AM, Avi Kivity wrote:
> However kvm_arch_vcpu_init(), which creates the lapic, _is_ called
> without either the vcpu->mutex or kvm->lock held.

This is irrelevant, the important bit is when it becomes visible.

>   Patch coming up.

I'll add explicit variables for irqchip_in_kernel.  They'll be useful
for non-x86 as well.

-- 
error compiling committee.c: too many arguments to function