* [PATCH] KVM: x86: Avoid NULL dereference in kvm_apic_accept_pic_intr() @ 2012-02-07 6:32 Michael Ellerman 2012-02-07 19:38 ` Marcelo Tosatti 0 siblings, 1 reply; 6+ messages in thread From: Michael Ellerman @ 2012-02-07 6:32 UTC (permalink / raw) To: kvm; +Cc: avi, mtosatti 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: x86: Avoid NULL dereference in kvm_apic_accept_pic_intr() 2012-02-07 6:32 [PATCH] KVM: x86: Avoid NULL dereference in kvm_apic_accept_pic_intr() Michael Ellerman @ 2012-02-07 19:38 ` Marcelo Tosatti 2012-02-08 10:41 ` Michael Ellerman 0 siblings, 1 reply; 6+ messages in thread From: Marcelo Tosatti @ 2012-02-07 19:38 UTC (permalink / raw) To: Michael Ellerman; +Cc: kvm, avi 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: x86: Avoid NULL dereference in kvm_apic_accept_pic_intr() 2012-02-07 19:38 ` Marcelo Tosatti @ 2012-02-08 10:41 ` Michael Ellerman 2012-02-08 12:13 ` Michael Ellerman 0 siblings, 1 reply; 6+ messages in thread From: Michael Ellerman @ 2012-02-08 10:41 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: kvm, avi [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: x86: Avoid NULL dereference in kvm_apic_accept_pic_intr() 2012-02-08 10:41 ` Michael Ellerman @ 2012-02-08 12:13 ` Michael Ellerman 2012-03-04 9:51 ` Avi Kivity 0 siblings, 1 reply; 6+ messages in thread From: Michael Ellerman @ 2012-02-08 12:13 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: kvm, avi [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: x86: Avoid NULL dereference in kvm_apic_accept_pic_intr() 2012-02-08 12:13 ` Michael Ellerman @ 2012-03-04 9:51 ` Avi Kivity 2012-03-04 10:14 ` Avi Kivity 0 siblings, 1 reply; 6+ messages in thread From: Avi Kivity @ 2012-03-04 9:51 UTC (permalink / raw) To: michael; +Cc: Marcelo Tosatti, kvm 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: x86: Avoid NULL dereference in kvm_apic_accept_pic_intr() 2012-03-04 9:51 ` Avi Kivity @ 2012-03-04 10:14 ` Avi Kivity 0 siblings, 0 replies; 6+ messages in thread From: Avi Kivity @ 2012-03-04 10:14 UTC (permalink / raw) To: michael; +Cc: Marcelo Tosatti, kvm 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-03-04 10:14 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-02-07 6:32 [PATCH] KVM: x86: Avoid NULL dereference in kvm_apic_accept_pic_intr() Michael Ellerman 2012-02-07 19:38 ` Marcelo Tosatti 2012-02-08 10:41 ` Michael Ellerman 2012-02-08 12:13 ` Michael Ellerman 2012-03-04 9:51 ` Avi Kivity 2012-03-04 10:14 ` Avi Kivity
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.