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: Sat, 14 Dec 2013 11:46:14 +0200 Message-ID: <20131214094614.GF21068@minantech.com> References: <1386880614-23300-4-git-send-email-pbonzini@redhat.com> <20131213160754.GA20763@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, gleb@redhat.com, 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: <20131213160754.GA20763@hpx.cz> Sender: stable-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On Fri, Dec 13, 2013 at 05:07:54PM +0100, Radim Kr=C4=8Dm=C3=A1=C5=99 w= rote: > 2013-12-12 21:36+0100, Paolo Bonzini: > > From: Gleb Natapov > >=20 > > A guest can cause a BUG_ON() leading to a host kernel crash. > > When the guest writes to the ICR to request an IPI, while in x2apic > > mode the following things happen, the destination is read from > > ICR2, which is a register that the guest can control. > >=20 > > kvm_irq_delivery_to_apic_fast uses the high 16 bits of ICR2 as the > > cluster id. A BUG_ON is triggered, which is a protection against > > accessing map->logical_map with an out-of-bounds access and manages > > to avoid that anything really unsafe occurs. > >=20 > > The logic in the code is correct from real HW point of view. The pr= oblem > > is that KVM supports only one cluster with ID 0 in clustered mode, = but > > the code that has the bug does not take this into account. >=20 > The more I read about x2apic, the more confused I am ... >=20 > - How was the cluster x2apic enabled? >=20 > Linux won't enable cluster x2apic without interrupt remapping and = I > had no idea we were allowed to do it. >=20 Malicious code can do it. > - A hardware test-suite found this? >=20 > This bug can only be hit when the destination cpu is > 256, so the > request itself is buggy -- we don't support that many in kvm and i= t > would crash when initializing the vcpus if we did. > =3D> It looks like we should just ignore the ipi, because we have = no > vcpus in that cluster. >=20 That's the nature of malicious code: it does what your code does not ex= pects it to do :) > - Where does the 'only one supported cluster' come from? >=20 "only one supported cluster" comes from 8 bit cpuid limitation of KVM's= x2apic implementation. With 8 bit cpuid you can only address cluster 0 in logi= cal mode. > 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 correctly in > the cluster x2apic, with > 16 vcpus. >=20 Such config cannot work today because of 8 bit cpuid limitation. When t= he limitation will be removed KMV_X2APIC_CID_BITS will be set to actual number of bit= s we want to support. It will likely be smaller then 16 bit though since full 18 bit support = will require huge tables. > Thanks. >=20 > > Reported-by: Lars Bull > > Cc: stable@vger.kernel.org > > Signed-off-by: Gleb Natapov > > Signed-off-by: Paolo Bonzini > > --- > > arch/x86/kvm/lapic.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > >=20 > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > index b8bec45c1610..801dc3fd66e1 100644 > > --- a/arch/x86/kvm/lapic.c > > +++ b/arch/x86/kvm/lapic.c > > @@ -143,6 +143,8 @@ static inline int kvm_apic_id(struct kvm_lapic = *apic) > > return (kvm_apic_get_reg(apic, APIC_ID) >> 24) & 0xff; > > } > > =20 > > +#define KMV_X2APIC_CID_BITS 0 > > + > > static void recalculate_apic_map(struct kvm *kvm) > > { > > struct kvm_apic_map *new, *old =3D NULL; > > @@ -180,7 +182,8 @@ static void recalculate_apic_map(struct kvm *kv= m) > > if (apic_x2apic_mode(apic)) { > > new->ldr_bits =3D 32; > > new->cid_shift =3D 16; > > - new->cid_mask =3D new->lid_mask =3D 0xffff; > > + new->cid_mask =3D (1 << KMV_X2APIC_CID_BITS) - 1; > > + new->lid_mask =3D 0xffff; > > } else if (kvm_apic_sw_enabled(apic) && > > !new->cid_mask /* flat mode */ && > > kvm_apic_get_reg(apic, APIC_DFR) =3D=3D APIC_DFR_CLUSTER) { > > --=20 > > 1.8.3.1 > >=20 > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-ker= nel" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ > -- > To unsubscribe from this list: send the line "unsubscribe linux-kerne= l" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- Gleb.