From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Hildenbrand Subject: Re: [PATCH v3] KVM: remove buggy vcpu id check on vcpu creation Date: Thu, 21 Apr 2016 15:22:55 +0200 Message-ID: <20160421152255.5b1a04af@thinkpad-w530> References: <146116689259.20666.15860134511726195550.stgit@bahia.huguette.org> <20160420182909.GB4044@potion> <20160421132958.0e9292d5@bahia.huguette.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Radim =?UTF-8?B?S3LEjW3DocWZ?= , Paolo Bonzini , james.hogan@imgtec.com, mingo@redhat.com, linux-mips@linux-mips.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, qemu-ppc@nongnu.org, Cornelia Huck , Paul Mackerras , David Gibson To: Greg Kurz Return-path: In-Reply-To: <20160421132958.0e9292d5@bahia.huguette.org> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org > On Wed, 20 Apr 2016 20:29:09 +0200 > Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: >=20 > > 2016-04-20 17:44+0200, Greg Kurz: =20 > > > Commit 338c7dbadd26 ("KVM: Improve create VCPU parameter (CVE-201= 3-4587)") > > > introduced a check to prevent potential kernel memory corruption = in case > > > the vcpu id is too great. > > >=20 > > > 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. > > >=20 > > > 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 candi= date. > > >=20 > > > ARM and s390 already have such a check. > > >=20 > > > 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 ca= n live > > > without this check. =20 > >=20 > > The only problematic path I see is kvm_get_vcpu_by_id(), which retu= rns > > NULL for any id above KVM_MAX_VCPUS. =20 >=20 > Oops my bad, I started to work on a 4.4 tree and I missed this check = brought > by commit c896939f7cff (KVM: use heuristic for fast VCPU lookup by id= ). >=20 > But again, I believe the check is wrong there also: the changelog jus= t mentions > this is a fastpath for the usual case where "VCPU ids match the array= index"... > why does the patch add a NULL return path if id >=3D KVM_MAX_VCPUS ? >=20 > > kvm_vm_ioctl_create_vcpu() uses kvm_get_vcpu_by_id() to check for > > duplicate ids, so PowerPC could end up with many VCPUs of the same = id. > > I'm not sure what could fail, but code doesn't expect this situatio= n. > > Patching kvm_get_vcpu_by_id() is easy, though. > > =20 >=20 > Something like this ? >=20 > if (id < 0) > return NULL; > if (id < KVM_MAX_VCPUS) > vcpu =3D kvm_get_vcpu(kvm, id); >=20 > In the same patch ? So the heuristic would only trigger if id < KVM_MAX_VCPUS. By initializing vcpu to NULL this would work. David