On Fri, Dec 13, 2013 at 05:07:54PM +0100, Radim Krčmář wrote:
> 2013-12-12 21:36+0100, Paolo Bonzini:
> > From: Gleb Natapov <gleb@xxxxxxxxxx>
> >
> > 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.
> >
> > 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.
> >
> > The logic in the code is correct from real HW point of view. The problem
> > 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.
>
> The more I read about x2apic, the more confused I am ...
>
> - How was the cluster x2apic enabled?
>
> Linux won't enable cluster x2apic without interrupt remapping and I
> had no idea we were allowed to do it.
>
Malicious code can do it.
> - A hardware test-suite found this?
>
> 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 it
> would crash when initializing the vcpus if we did.
> => It looks like we should just ignore the ipi, because we have no
> vcpus in that cluster.
>
That's the nature of malicious code: it does what your code does not expects
it to do :)
> - Where does the 'only one supported cluster' come from?
>
"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 logical mode.
> I only see we use 'struct kvm_lapic *logical_map[16][16];', which
> supports 16 clusters of 16 apics = 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.
>
Such config cannot work today because of 8 bit cpuid limitation. When the limitation
will be removed KMV_X2APIC_CID_BITS will be set to actual number of bits we want to support.
It will likely be smaller then 16 bit though since full 18 bit support will require huge tables.
> Thanks.
>
> > Reported-by: Lars Bull <larsbull@xxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Gleb Natapov <gleb@xxxxxxxxxx>
> > Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > ---
> > arch/x86/kvm/lapic.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > 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;
> > }
> >
> > +#define KMV_X2APIC_CID_BITS 0
> > +
> > static void recalculate_apic_map(struct kvm *kvm)
> > {
> > struct kvm_apic_map *new, *old = NULL;
> > @@ -180,7 +182,8 @@ static void recalculate_apic_map(struct kvm *kvm)
> > if (apic_x2apic_mode(apic)) {
> > new->ldr_bits = 32;
> > new->cid_shift = 16;
> > - new->cid_mask = new->lid_mask = 0xffff;
> > + new->cid_mask = (1 << KMV_X2APIC_CID_BITS) - 1;
> > + new->lid_mask = 0xffff;
> > } else if (kvm_apic_sw_enabled(apic) &&
> > !new->cid_mask /* flat mode */ &&
> > kvm_apic_get_reg(apic, APIC_DFR) == APIC_DFR_CLUSTER) {
> > --
> > 1.8.3.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > 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-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html