Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754276AbcDVJa6 (ORCPT ); Fri, 22 Apr 2016 05:30:58 -0400 Received: from e06smtp06.uk.ibm.com ([195.75.94.102]:42740 "EHLO e06smtp06.uk.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752113AbcDVJa5 (ORCPT ); Fri, 22 Apr 2016 05:30:57 -0400 X-IBM-Helo: d06dlp02.portsmouth.uk.ibm.com X-IBM-MailFrom: gkurz@linux.vnet.ibm.com X-IBM-RcptTo: kvm@vger.kernel.org;linux-kernel@vger.kernel.org Date: Fri, 22 Apr 2016 11:30:45 +0200 From: Greg Kurz To: Wei Yang Cc: Paolo Bonzini , , , , , , , David Hildenbrand , , Cornelia Huck , Paul Mackerras , David Gibson Subject: Re: [PATCH v4 2/2] KVM: move vcpu id checking to archs Message-ID: <20160422113045.14560b66@bahia.huguette.org> In-Reply-To: <20160422092103.GA3851@linux-gk3p> References: <146124809455.32509.15232948272580716135.stgit@bahia.huguette.org> <146124811255.32509.17679765789502091772.stgit@bahia.huguette.org> <20160422092103.GA3851@linux-gk3p> Organization: IBM X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16042209-0025-0000-0000-00001408220F Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4970 Lines: 137 On Fri, 22 Apr 2016 17:21:03 +0800 Wei Yang wrote: > Hi, Greg > Hi Wei ! > One confusion. > > There are 5 kvm_arch_vcpu_create() while in this patch you changed 2 of them. > Some particular reason? > Yes and the reason is given in the changelog: - ARM and s390 already have such a check - PowerPC can live without this check - this patch simply moves the check to MIPS and x86 Does it clarify ? Cheers. -- Greg > On Thu, Apr 21, 2016 at 04:20:53PM +0200, Greg Kurz 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. While here, > >we also update the documentation to dissociate vcpu ids from the maximum > >number of vcpus per virtual machine. > > > >Acked-by: James Hogan > >Acked-by: Cornelia Huck > >Signed-off-by: Greg Kurz > >--- > >v4: - updated subject for more clarity on what the patch does > > - added James's and Connie's A-b tags > > - updated documentation > > > > Documentation/virtual/kvm/api.txt | 7 +++---- > > arch/mips/kvm/mips.c | 7 ++++++- > > arch/x86/kvm/x86.c | 3 +++ > > virt/kvm/kvm_main.c | 3 --- > > 4 files changed, 12 insertions(+), 8 deletions(-) > > > >diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt > >index 4d0542c5206b..486a1d783b82 100644 > >--- a/Documentation/virtual/kvm/api.txt > >+++ b/Documentation/virtual/kvm/api.txt > >@@ -199,11 +199,10 @@ Type: vm ioctl > > Parameters: vcpu id (apic id on x86) > > Returns: vcpu fd on success, -1 on error > > > >-This API adds a vcpu to a virtual machine. The vcpu id is a small integer > >-in the range [0, max_vcpus). > >+This API adds a vcpu to a virtual machine. The vcpu id is a positive integer. > > > >-The recommended max_vcpus value can be retrieved using the KVM_CAP_NR_VCPUS of > >-the KVM_CHECK_EXTENSION ioctl() at run-time. > >+The recommended maximum number of vcpus (max_vcpus) can be retrieved using the > >+KVM_CAP_NR_VCPUS of the KVM_CHECK_EXTENSION ioctl() at run-time. > > The maximum possible value for max_vcpus can be retrieved using the > > KVM_CAP_MAX_VCPUS of the KVM_CHECK_EXTENSION ioctl() at run-time. > > > >diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c > >index 70ef1a43c114..0278ea146db5 100644 > >--- a/arch/mips/kvm/mips.c > >+++ b/arch/mips/kvm/mips.c > >@@ -248,9 +248,14 @@ 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) { > >+ err = -EINVAL; > >+ goto out; > >+ } > > > >+ 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..7738202edcce 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 ERR_PTR(-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 >