From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH] KVM: x86: fix guest-initiated crash with x2apic (CVE-2013-6376) Date: Mon, 16 Dec 2013 18:22:42 +0200 Message-ID: <20131216162242.GH21068@minantech.com> References: <1386880614-23300-4-git-send-email-pbonzini@redhat.com> <20131213160754.GA20763@hpx.cz> <20131214094614.GF21068@minantech.com> <20131216120109.GA3324@hpx.cz> <20131216121637.GB24218@minantech.com> <20131216125526.GC3324@hpx.cz> <20131216133142.GA3676@hpx.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Paolo Bonzini , linux-kernel@vger.kernel.org, kvm@vger.kernel.org, pmatouse@redhat.com, stable@vger.kernel.org, larsbull@google.com To: Radim =?utf-8?B?S3LEjW3DocWZ?= Return-path: Content-Disposition: inline In-Reply-To: <20131216133142.GA3676@hpx.cz> Sender: stable-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On Mon, Dec 16, 2013 at 02:31:43PM +0100, Radim Kr=C4=8Dm=C3=A1=C5=99 w= rote: > 2013-12-16 13:55+0100, Radim Kr=C4=8Dm=C3=A1=C5=99: > > 2013-12-16 14:16+0200, Gleb Natapov: > > > On Mon, Dec 16, 2013 at 01:01:10PM +0100, Radim Kr=C4=8Dm=C3=A1=C5= =99 wrote: > > > > > > - Where does the 'only one supported cluster' come from? > > > > > >=20 > > > > > "only one supported cluster" comes from 8 bit cpuid limitatio= n of KVM's x2apic > > > > > implementation. With 8 bit cpuid you can only address cluster= 0 in logical mode. > > > >=20 > > > > One x2apic cluster has 16 cpus and we generate the x2apic LDR c= orrectly, > > > > so 8 bit cpuid can address first 16 clusters as well. > > > >=20 > > > > u32 ldr =3D ((id >> 4) << 16) | (1 << (id & 0xf)); > > > >=20 > > > Interrupt from a device cannot generate such ldr, only IPI can. O= nly > > > 4 cpus in cluster zero are addressable in clustering mode by a > > > device. Without irq remapping x2apic is a PV interface between ho= st > > > and guest where guest needs to know KVM implementation's limitati= on to > > > use it. > >=20 > > Thanks, I'll read more about devices ... still no idea how could th= ey > > address cluster > 15. > >=20 With irq remapping device they will be able to provide 32bit apic addre= sses. > > > I do not see a point in fixing problems in x2apic logical= mode > > > emulation right now since it will not make it usable, as long as > > > there is not security problems there. > >=20 > > Agreed; I wanted to know why this patch was correct, if we cared. > >=20 > > > > > > I only see we use 'struct kvm_lapic *logical_map[16][16]= ;', which > > > > > > supports 16 clusters of 16 apics =3D first 256 vcpus, so= if we map > > > > > > everything to logical_map[0][0:15], we would not work co= rrectly in > > > > > > the cluster x2apic, with > 16 vcpus. > > > > > >=20 > > > > > Such config cannot work today because of 8 bit cpuid limitati= on. When the limitation > > > > > will be removed KMV_X2APIC_CID_BITS will be set to actual num= ber of bits we want to support. > > > >=20 > > > > Even with KMV_X2APIC_CID_BITS =3D 4, which would allow us to su= pport 8 bit > > > > cpuid, we would still deliver interrupts destined for cpuid > 2= 56 to > > > > potentially plugged cpus. > > > Again, KMV_X2APIC_CID_BITS =3D 4 will not allow us to support 8 b= it cpuids > > > unfortunately, not sure what you mean by the second part of the s= entence. > >=20 > > Sorry, I meant that with this change, we map all clusters to cluste= r 0, > > which has two flaws: > > - in kvm_lapic_set_base(), the order of vcpu creation determines t= hose >=20 > Gah, should have been 'recalculate_apic_map()' ... >=20 > The patch would be especially surprising with a dynamic adding of vcp= us. >=20 > > assigned to cluster 0, and the rest is unaddressable (overwritte= n) > > - we can send IPI to an unplugged high cpuid and it arives in clus= ter 0 It does not matter since well behaved guest is not supposed to configur= e apic like that. -- Gleb.