| Message ID | 146116593847.17621.12790270691583056759.stgit@bahia.huguette.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Forget this post, I'll send a v3. Thanks to the kbuild test robot ! :) On Wed, 20 Apr 2016 17:26:48 +0200 Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > Commit 338c7dbadd26 ("KVM: Improve create VCPU parameter (CVE-2013-4587)") > introduced a check to prevent potential kernel memory corruption in case > the vcpu id is too great. > > Unfortunately this check assumes vcpu ids grow in sequence with a common > difference of 1, which is wrong: archs are free to use vcpu id as they fit. > For example, QEMU originated vcpu ids for PowerPC cpus running in boot3s_hv > mode, can grow with a common difference of 2, 4 or 8: if KVM_MAX_VCPUS is > 1024, guests may be limited down to 128 vcpus on POWER8. > > This means the check does not belong here and should be moved to some arch > specific function: kvm_arch_vcpu_create() looks like a good candidate. > > ARM and s390 already have such a check. > > I could not spot any path in the PowerPC or common KVM code where a vcpu > id is used as described in the above commit: I believe PowerPC can live > without this check. > > In the end, this patch simply moves the check to MIPS and x86. > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > --- > v2: move kzalloc() after the check in MIPS to avoid memory leak > > arch/mips/kvm/mips.c | 5 ++++- > arch/x86/kvm/x86.c | 3 +++ > virt/kvm/kvm_main.c | 3 --- > 3 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c > index 70ef1a43c114..c1ce9d44d685 100644 > --- a/arch/mips/kvm/mips.c > +++ b/arch/mips/kvm/mips.c > @@ -248,9 +248,12 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int id) > int err, size, offset; > void *gebase; > int i; > + struct kvm_vcpu *vcpu; > > - struct kvm_vcpu *vcpu = kzalloc(sizeof(struct kvm_vcpu), GFP_KERNEL); > + if (id >= KVM_MAX_VCPUS) > + return -EINVAL; > > + vcpu = kzalloc(sizeof(struct kvm_vcpu), GFP_KERNEL); > if (!vcpu) { > err = -ENOMEM; > goto out; > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 9b7798c7b210..f705d57b12ed 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -7358,6 +7358,9 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, > { > struct kvm_vcpu *vcpu; > > + if (id >= KVM_MAX_VCPUS) > + return -EINVAL; > + > if (check_tsc_unstable() && atomic_read(&kvm->online_vcpus) != 0) > printk_once(KERN_WARNING > "kvm: SMP vm created on host with unstable TSC; " > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 4fd482fb9260..6b6cca3cb488 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -2272,9 +2272,6 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id) > int r; > struct kvm_vcpu *vcpu; > > - if (id >= KVM_MAX_VCPUS) > - return -EINVAL; > - > vcpu = kvm_arch_vcpu_create(kvm, id); > if (IS_ERR(vcpu)) > return PTR_ERR(vcpu); > > -- 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
On Wed, 20 Apr 2016 17:26:48 +0200 Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > Commit 338c7dbadd26 ("KVM: Improve create VCPU parameter (CVE-2013-4587)") > introduced a check to prevent potential kernel memory corruption in case > the vcpu id is too great. > > Unfortunately this check assumes vcpu ids grow in sequence with a common > difference of 1, which is wrong: archs are free to use vcpu id as they fit. > For example, QEMU originated vcpu ids for PowerPC cpus running in boot3s_hv > mode, can grow with a common difference of 2, 4 or 8: if KVM_MAX_VCPUS is > 1024, guests may be limited down to 128 vcpus on POWER8. > > This means the check does not belong here and should be moved to some arch > specific function: kvm_arch_vcpu_create() looks like a good candidate. > > ARM and s390 already have such a check. > > I could not spot any path in the PowerPC or common KVM code where a vcpu > id is used as described in the above commit: I believe PowerPC can live > without this check. > > In the end, this patch simply moves the check to MIPS and x86. The concept makes sense to me... > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > --- > v2: move kzalloc() after the check in MIPS to avoid memory leak > > arch/mips/kvm/mips.c | 5 ++++- > arch/x86/kvm/x86.c | 3 +++ > virt/kvm/kvm_main.c | 3 --- > 3 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c > index 70ef1a43c114..c1ce9d44d685 100644 > --- a/arch/mips/kvm/mips.c > +++ b/arch/mips/kvm/mips.c > @@ -248,9 +248,12 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int id) > int err, size, offset; > void *gebase; > int i; > + struct kvm_vcpu *vcpu; > > - struct kvm_vcpu *vcpu = kzalloc(sizeof(struct kvm_vcpu), GFP_KERNEL); > + if (id >= KVM_MAX_VCPUS) > + return -EINVAL; ...but you need to use ERR_PTR(). > > + vcpu = kzalloc(sizeof(struct kvm_vcpu), GFP_KERNEL); > if (!vcpu) { > err = -ENOMEM; > goto out; > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 9b7798c7b210..f705d57b12ed 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -7358,6 +7358,9 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, > { > struct kvm_vcpu *vcpu; > > + if (id >= KVM_MAX_VCPUS) > + return -EINVAL; Here as well. > + > if (check_tsc_unstable() && atomic_read(&kvm->online_vcpus) != 0) > printk_once(KERN_WARNING > "kvm: SMP vm created on host with unstable TSC; " -- 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
diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c index 70ef1a43c114..c1ce9d44d685 100644 --- a/arch/mips/kvm/mips.c +++ b/arch/mips/kvm/mips.c @@ -248,9 +248,12 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int id) int err, size, offset; void *gebase; int i; + struct kvm_vcpu *vcpu; - struct kvm_vcpu *vcpu = kzalloc(sizeof(struct kvm_vcpu), GFP_KERNEL); + if (id >= KVM_MAX_VCPUS) + return -EINVAL; + vcpu = kzalloc(sizeof(struct kvm_vcpu), GFP_KERNEL); if (!vcpu) { err = -ENOMEM; goto out; diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 9b7798c7b210..f705d57b12ed 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -7358,6 +7358,9 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, { struct kvm_vcpu *vcpu; + if (id >= KVM_MAX_VCPUS) + return -EINVAL; + if (check_tsc_unstable() && atomic_read(&kvm->online_vcpus) != 0) printk_once(KERN_WARNING "kvm: SMP vm created on host with unstable TSC; " diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 4fd482fb9260..6b6cca3cb488 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -2272,9 +2272,6 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id) int r; struct kvm_vcpu *vcpu; - if (id >= KVM_MAX_VCPUS) - return -EINVAL; - vcpu = kvm_arch_vcpu_create(kvm, id); if (IS_ERR(vcpu)) return PTR_ERR(vcpu);
Commit 338c7dbadd26 ("KVM: Improve create VCPU parameter (CVE-2013-4587)") introduced a check to prevent potential kernel memory corruption in case the vcpu id is too great. Unfortunately this check assumes vcpu ids grow in sequence with a common difference of 1, which is wrong: archs are free to use vcpu id as they fit. For example, QEMU originated vcpu ids for PowerPC cpus running in boot3s_hv mode, can grow with a common difference of 2, 4 or 8: if KVM_MAX_VCPUS is 1024, guests may be limited down to 128 vcpus on POWER8. This means the check does not belong here and should be moved to some arch specific function: kvm_arch_vcpu_create() looks like a good candidate. ARM and s390 already have such a check. I could not spot any path in the PowerPC or common KVM code where a vcpu id is used as described in the above commit: I believe PowerPC can live without this check. In the end, this patch simply moves the check to MIPS and x86. Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> --- v2: move kzalloc() after the check in MIPS to avoid memory leak arch/mips/kvm/mips.c | 5 ++++- arch/x86/kvm/x86.c | 3 +++ virt/kvm/kvm_main.c | 3 --- 3 files changed, 7 insertions(+), 4 deletions(-) -- 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