Message ID | 4F994C8A.8090909@canonical.com |
---|---|
State | New |
Headers | show |
On 26.04.2012 15:24, Tim Gardner wrote: > On 04/26/2012 06:57 AM, Stefan Bader wrote: >> On 26.04.2012 14:52, Tim Gardner wrote: >>> So this patch causes a compile error. I've attached the correction. I >>> suggest that it just be squashed into the original commit. >>> >>> rtg >>> >>> >>> >>> >> Maybe we want to pick up the patch that introduced it: >> >> >> commit d780592b99d7d8a5ff905f6bacca519d4a342c76 >> Author: Jan Kiszka <jan.kiszka@siemens.com> >> Date: Mon May 23 10:33:05 2011 +0200 >> >> KVM: Clean up error handling during VCPU creation >> >> Not sure all the jumps to vcpu_destroy before would otherwise do the right thing... >> >> >> >> > > OK, here is the real solution. d780592b99d7d8a5ff905f6bacca519d4a342c76 > and 3e515705a1f46beb1c942bb8043c16f8ac7b1e9e are clean cherry-picks. No > backport required. > > rtg fwiw... and likely going back...
On Thu, Apr 26, 2012 at 07:24:26AM -0600, Tim Gardner wrote: > On 04/26/2012 06:57 AM, Stefan Bader wrote: > > On 26.04.2012 14:52, Tim Gardner wrote: > >> So this patch causes a compile error. I've attached the correction. I > >> suggest that it just be squashed into the original commit. > >> > >> rtg > >> > >> > >> > >> > > Maybe we want to pick up the patch that introduced it: > > > > > > commit d780592b99d7d8a5ff905f6bacca519d4a342c76 > > Author: Jan Kiszka <jan.kiszka@siemens.com> > > Date: Mon May 23 10:33:05 2011 +0200 > > > > KVM: Clean up error handling during VCPU creation > > > > Not sure all the jumps to vcpu_destroy before would otherwise do the right thing... > > > > > > > > > > OK, here is the real solution. d780592b99d7d8a5ff905f6bacca519d4a342c76 > and 3e515705a1f46beb1c942bb8043c16f8ac7b1e9e are clean cherry-picks. No > backport required. > > rtg > -- > Tim Gardner tim.gardner@canonical.com > From cf94f3808929793e07361d2b7bc6edb44969d47f Mon Sep 17 00:00:00 2001 > From: Jan Kiszka <jan.kiszka@siemens.com> > Date: Mon, 23 May 2011 10:33:05 +0200 > Subject: [PATCH 1/2] KVM: Clean up error handling during VCPU creation > > BugLink: http://bugs.launchpad.net/bugs/971685 > > CVE-2012-1601 > > So far kvm_arch_vcpu_setup is responsible for freeing the vcpu struct if > it fails. Move this confusing resonsibility back into the hands of > kvm_vm_ioctl_create_vcpu. Only kvm_arch_vcpu_setup of x86 is affected, > all other archs cannot fail. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > Signed-off-by: Avi Kivity <avi@redhat.com> > (cherry picked from commit d780592b99d7d8a5ff905f6bacca519d4a342c76) > > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > --- > arch/x86/kvm/x86.c | 5 ----- > virt/kvm/kvm_main.c | 11 ++++++----- > 2 files changed, 6 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 8e15578..d829351 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -6145,12 +6145,7 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu) > if (r == 0) > r = kvm_mmu_setup(vcpu); > vcpu_put(vcpu); > - if (r < 0) > - goto free_vcpu; > > - return 0; > -free_vcpu: > - kvm_x86_ops->vcpu_free(vcpu); > return r; > } > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 96ebc06..7d6b8e3 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -1615,18 +1615,18 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id) > > r = kvm_arch_vcpu_setup(vcpu); > if (r) > - return r; > + goto vcpu_destroy; > > mutex_lock(&kvm->lock); > if (atomic_read(&kvm->online_vcpus) == KVM_MAX_VCPUS) { > r = -EINVAL; > - goto vcpu_destroy; > + goto unlock_vcpu_destroy; > } > > kvm_for_each_vcpu(r, v, kvm) > if (v->vcpu_id == id) { > r = -EEXIST; > - goto vcpu_destroy; > + goto unlock_vcpu_destroy; > } > > BUG_ON(kvm->vcpus[atomic_read(&kvm->online_vcpus)]); > @@ -1636,7 +1636,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id) > r = create_vcpu_fd(vcpu); > if (r < 0) { > kvm_put_kvm(kvm); > - goto vcpu_destroy; > + goto unlock_vcpu_destroy; > } > > kvm->vcpus[atomic_read(&kvm->online_vcpus)] = vcpu; > @@ -1650,8 +1650,9 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id) > mutex_unlock(&kvm->lock); > return r; > > -vcpu_destroy: > +unlock_vcpu_destroy: > mutex_unlock(&kvm->lock); > +vcpu_destroy: > kvm_arch_vcpu_destroy(vcpu); > return r; > } > -- > 1.7.9.5 > > From 08a5694c9cce61f44bc4909ecb947586105374a3 Mon Sep 17 00:00:00 2001 > From: Avi Kivity <avi@redhat.com> > Date: Mon, 5 Mar 2012 14:23:29 +0200 > Subject: [PATCH 2/2] KVM: Ensure all vcpus are consistent with in-kernel > irqchip settings > > BugLink: http://bugs.launchpad.net/bugs/971685 > > CVE-2012-1601 > > If some vcpus are created before KVM_CREATE_IRQCHIP, then > irqchip_in_kernel() and vcpu->arch.apic will be inconsistent, leading > to potential NULL pointer dereferences. > > Fix by: > - ensuring that no vcpus are installed when KVM_CREATE_IRQCHIP is called > - ensuring that a vcpu has an apic if it is installed after KVM_CREATE_IRQCHIP > > This is somewhat long winded because vcpu->arch.apic is created without > kvm->lock held. > > Based on earlier patch by Michael Ellerman. > > Signed-off-by: Michael Ellerman <michael@ellerman.id.au> > Signed-off-by: Avi Kivity <avi@redhat.com> > (cherry picked from commit 3e515705a1f46beb1c942bb8043c16f8ac7b1e9e) > > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > --- > arch/ia64/kvm/kvm-ia64.c | 5 +++++ > arch/x86/kvm/x86.c | 8 ++++++++ > include/linux/kvm_host.h | 7 +++++++ > virt/kvm/kvm_main.c | 4 ++++ > 4 files changed, 24 insertions(+) > > diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c > index 8213efe..a874213 100644 > --- a/arch/ia64/kvm/kvm-ia64.c > +++ b/arch/ia64/kvm/kvm-ia64.c > @@ -1168,6 +1168,11 @@ out: > > #define PALE_RESET_ENTRY 0x80000000ffffffb0UL > > +bool kvm_vcpu_compatible(struct kvm_vcpu *vcpu) > +{ > + return irqchip_in_kernel(vcpu->kcm) == (vcpu->arch.apic != NULL); > +} > + > int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu) > { > struct kvm_vcpu *v; > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index d829351..bf59dfc 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -3439,6 +3439,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) { > @@ -6218,6 +6221,11 @@ void kvm_arch_check_processor_compat(void *rtn) > kvm_x86_ops->check_processor_compatibility(rtn); > } > > +bool kvm_vcpu_compatible(struct kvm_vcpu *vcpu) > +{ > + return irqchip_in_kernel(vcpu->kvm) == (vcpu->arch.apic != NULL); > +} > + > int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu) > { > struct page *page; > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 31ebb59..d70a250 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -730,6 +730,13 @@ static inline bool kvm_vcpu_is_bsp(struct kvm_vcpu *vcpu) > { > return vcpu->kvm->bsp_vcpu_id == vcpu->vcpu_id; > } > + > +bool kvm_vcpu_compatible(struct kvm_vcpu *vcpu); > + > +#else > + > +static inline bool kvm_vcpu_compatible(struct kvm_vcpu *vcpu) { return true; } > + > #endif > > #ifdef __KVM_HAVE_DEVICE_ASSIGNMENT > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 7d6b8e3..f9013d1 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -1618,6 +1618,10 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id) > goto vcpu_destroy; > > mutex_lock(&kvm->lock); > + if (!kvm_vcpu_compatible(vcpu)) { > + r = -EINVAL; > + goto unlock_vcpu_destroy; > + } > if (atomic_read(&kvm->online_vcpus) == KVM_MAX_VCPUS) { > r = -EINVAL; > goto unlock_vcpu_destroy; > -- > 1.7.9.5 > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
From 08a5694c9cce61f44bc4909ecb947586105374a3 Mon Sep 17 00:00:00 2001 From: Avi Kivity <avi@redhat.com> Date: Mon, 5 Mar 2012 14:23:29 +0200 Subject: [PATCH 2/2] KVM: Ensure all vcpus are consistent with in-kernel irqchip settings BugLink: http://bugs.launchpad.net/bugs/971685 CVE-2012-1601 If some vcpus are created before KVM_CREATE_IRQCHIP, then irqchip_in_kernel() and vcpu->arch.apic will be inconsistent, leading to potential NULL pointer dereferences. Fix by: - ensuring that no vcpus are installed when KVM_CREATE_IRQCHIP is called - ensuring that a vcpu has an apic if it is installed after KVM_CREATE_IRQCHIP This is somewhat long winded because vcpu->arch.apic is created without kvm->lock held. Based on earlier patch by Michael Ellerman. Signed-off-by: Michael Ellerman <michael@ellerman.id.au> Signed-off-by: Avi Kivity <avi@redhat.com> (cherry picked from commit 3e515705a1f46beb1c942bb8043c16f8ac7b1e9e) Signed-off-by: Tim Gardner <tim.gardner@canonical.com> --- arch/ia64/kvm/kvm-ia64.c | 5 +++++ arch/x86/kvm/x86.c | 8 ++++++++ include/linux/kvm_host.h | 7 +++++++ virt/kvm/kvm_main.c | 4 ++++ 4 files changed, 24 insertions(+) diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c index 8213efe..a874213 100644 --- a/arch/ia64/kvm/kvm-ia64.c +++ b/arch/ia64/kvm/kvm-ia64.c @@ -1168,6 +1168,11 @@ out: #define PALE_RESET_ENTRY 0x80000000ffffffb0UL +bool kvm_vcpu_compatible(struct kvm_vcpu *vcpu) +{ + return irqchip_in_kernel(vcpu->kcm) == (vcpu->arch.apic != NULL); +} + int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu) { struct kvm_vcpu *v; diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index d829351..bf59dfc 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3439,6 +3439,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) { @@ -6218,6 +6221,11 @@ void kvm_arch_check_processor_compat(void *rtn) kvm_x86_ops->check_processor_compatibility(rtn); } +bool kvm_vcpu_compatible(struct kvm_vcpu *vcpu) +{ + return irqchip_in_kernel(vcpu->kvm) == (vcpu->arch.apic != NULL); +} + int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu) { struct page *page; diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 31ebb59..d70a250 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -730,6 +730,13 @@ static inline bool kvm_vcpu_is_bsp(struct kvm_vcpu *vcpu) { return vcpu->kvm->bsp_vcpu_id == vcpu->vcpu_id; } + +bool kvm_vcpu_compatible(struct kvm_vcpu *vcpu); + +#else + +static inline bool kvm_vcpu_compatible(struct kvm_vcpu *vcpu) { return true; } + #endif #ifdef __KVM_HAVE_DEVICE_ASSIGNMENT diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 7d6b8e3..f9013d1 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1618,6 +1618,10 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id) goto vcpu_destroy; mutex_lock(&kvm->lock); + if (!kvm_vcpu_compatible(vcpu)) { + r = -EINVAL; + goto unlock_vcpu_destroy; + } if (atomic_read(&kvm->online_vcpus) == KVM_MAX_VCPUS) { r = -EINVAL; goto unlock_vcpu_destroy; -- 1.7.9.5