From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH] KVM: x86: fix guest-initiated crash with x2apic (CVE-2013-6376) Date: Fri, 13 Dec 2013 18:25:20 +0100 Message-ID: <52AB4300.9010006@redhat.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: linux-kernel@vger.kernel.org, Gleb Natapov , kvm@vger.kernel.org, pmatouse@redhat.com, stable@vger.kernel.org, larsbull@google.com To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= Return-path: In-Reply-To: <20131213160754.GA20763@hpx.cz> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org Il 13/12/2013 17:07, Radim Kr=C4=8Dm=C3=A1=C5=99 ha scritto: > 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. That's what should happen in physical mode. Something like this patch: diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 5439117d5c4c..1f8e9e1abd3b 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -613,9 +615,10 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm= , struct kvm_lapic *src, =20 if (irq->dest_mode =3D=3D 0) { /* physical mode */ if (irq->delivery_mode =3D=3D APIC_DM_LOWEST || - irq->dest_id =3D=3D 0xff) + irq->dest_id =3D=3D 0xff || + (apic_x2apic_mode(src) && irq->dest_id > 0xff)) goto out; - dst =3D &map->phys_map[irq->dest_id & 0xff]; + dst =3D &map->phys_map[irq->dest_id]; } else { u32 mda =3D irq->dest_id << (32 - map->ldr_bits); =20 On top of this, the x2apic spec documents a "broadcast" destination ID = that could be implemented as follows. But I have no idea if this is correct= and how it differs from the usual broadcast shorthand: @@ -815,9 +818,11 @@ static void apic_send_ipi(struct kvm_lapic *apic) irq.level =3D icr_low & APIC_INT_ASSERT; irq.trig_mode =3D icr_low & APIC_INT_LEVELTRIG; irq.shorthand =3D icr_low & APIC_SHORT_MASK; - if (apic_x2apic_mode(apic)) + if (apic_x2apic_mode(apic)) { irq.dest_id =3D icr_high; - else + if (icr_high =3D=3D 0xFFFFFFFF) + irq.shorthand =3D APIC_DEST_ALLINC; + } else irq.dest_id =3D GET_APIC_DEST_FIELD(icr_high); =20 trace_kvm_apic_ipi(icr_low, irq.dest_id); > - Where does the 'only one supported cluster' come from? >=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 correctly in > the cluster x2apic, with > 16 vcpus. I think you're right. Something like this would be a better fix: diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index dec48bfaddb8..58745cbbb7e6 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -198,7 +198,7 @@ static void recalculate_apic_map(struct kvm *kvm) cid =3D apic_cluster_id(new, ldr); lid =3D apic_logical_id(new, ldr); =20 - if (lid) + if (cid < ARRAY_SIZE(new->logical_map) && lid) new->logical_map[cid][ffs(lid) - 1] =3D apic; } out: @@ -621,9 +621,12 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm= , struct kvm_lapic *src, dst =3D &map->phys_map[irq->dest_id & 0xff]; } else { u32 mda =3D irq->dest_id << (32 - map->ldr_bits); + u32 cid =3D apic_cluster_id(map, mda); =20 - dst =3D map->logical_map[apic_cluster_id(map, mda)]; + if (cid >=3D ARRAY_SIZE(map->logical_map)) + goto out; =20 + dst =3D map->logical_map[cid]; bitmap =3D apic_logical_id(map, mda); =20 if (irq->delivery_mode =3D=3D APIC_DM_LOWEST) { diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h index c8b0d0d2da5c..5ccb71658894 100644 --- a/arch/x86/kvm/lapic.h +++ b/arch/x86/kvm/lapic.h @@ -152,8 +152,6 @@ static inline u16 apic_cluster_id(struct kvm_apic_m= ap *map, u32 ldr) ldr >>=3D 32 - map->ldr_bits; cid =3D (ldr >> map->cid_shift) & map->cid_mask; =20 - BUG_ON(cid >=3D ARRAY_SIZE(map->logical_map)); - return cid; } =20 playground:~/work/upstream/linux-2.6/arch/x86 pbonzini$ vi kvm/lapic.c playground:~/work/upstream/linux-2.6/arch/x86 pbonzini$ git diff !$ git diff kvm/lapic.c diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index dec48bfaddb8..1005185972a9 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -143,8 +143,6 @@ static inline int kvm_apic_id(struct kvm_lapic *api= c) return (kvm_apic_get_reg(apic, APIC_ID) >> 24) & 0xff; } =20 -#define KVM_X2APIC_CID_BITS 0 - static void recalculate_apic_map(struct kvm *kvm) { struct kvm_apic_map *new, *old =3D NULL; @@ -182,8 +180,7 @@ static void recalculate_apic_map(struct kvm *kvm) if (apic_x2apic_mode(apic)) { new->ldr_bits =3D 32; new->cid_shift =3D 16; - new->cid_mask =3D (1 << KVM_X2APIC_CID_BITS) - 1; - new->lid_mask =3D 0xffff; + new->cid_mask =3D 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) { @@ -198,7 +195,7 @@ static void recalculate_apic_map(struct kvm *kvm) cid =3D apic_cluster_id(new, ldr); lid =3D apic_logical_id(new, ldr); =20 - if (lid) + if (cid < ARRAY_SIZE(new->logical_map) && lid) new->logical_map[cid][ffs(lid) - 1] =3D apic; } out: @@ -621,9 +618,12 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm= , struct kvm_lapic *src, dst =3D &map->phys_map[irq->dest_id & 0xff]; } else { u32 mda =3D irq->dest_id << (32 - map->ldr_bits); + u32 cid =3D apic_cluster_id(map, mda); =20 - dst =3D map->logical_map[apic_cluster_id(map, mda)]; + if (cid >=3D ARRAY_SIZE(map->logical_map)) + goto out; =20 + dst =3D map->logical_map[cid]; bitmap =3D apic_logical_id(map, mda); =20 if (irq->delivery_mode =3D=3D APIC_DM_LOWEST) {