[PATCH 0/4] KVM: x86: APIC fixes

[PATCH 0/4] KVM: x86: APIC fixes

[Radim Krčmář]

The interesting one is [3/4], which improves upon a previous CVE fix;
we also handle logical destination wrapping in it, so [2/4] does the
same for physical;  and to make it nicer, [1/4] removes a condition.
[4/4] makes our fast path return true when the message was handled.

Radim Krčmář (4):
  KVM: x86: deliver phys lowest-prio
  KVM: x86: fix APIC physical destination wrapping
  KVM: x86: allow 256 logical x2APICs again
  KVM: x86: don't retry hopeless APIC delivery

 arch/x86/kvm/lapic.c | 20 +++++++++++---------
 arch/x86/kvm/lapic.h |  2 --
 2 files changed, 11 insertions(+), 11 deletions(-)

-- 
2.1.0

[PATCH 1/4] KVM: x86: deliver phys lowest-prio

[Radim Krčmář]

Physical mode can't address more than one APIC, but lowest-prio is
allowed, so we just reuse our paths.

SDM 10.6.2.1 Physical Destination:
  Also, for any non-broadcast IPI or I/O subsystem initiated interrupt
  with lowest priority delivery mode, software must ensure that APICs
  defined in the interrupt address are present and enabled to receive
  interrupts.

We could warn on top of that.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/lapic.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index e0e5642..e8ad09d 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -691,8 +691,6 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 		goto out;
 
 	if (irq->dest_mode == 0) { /* physical mode */
-		if (irq->delivery_mode == APIC_DM_LOWEST)
-			goto out;
 		dst = &map->phys_map[irq->dest_id & 0xff];
 	} else {
 		u32 mda = irq->dest_id << (32 - map->ldr_bits);
-- 
2.1.0

[PATCH 2/4] KVM: x86: fix APIC physical destination wrapping

[Radim Krčmář]

x2apic allows destinations > 0xff and we don't want them delivered to
lower APICs.  They are correctly handled by doing nothing.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 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 e8ad09d..049d30f 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -691,7 +691,10 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 		goto out;
 
 	if (irq->dest_mode == 0) { /* physical mode */
-		dst = &map->phys_map[irq->dest_id & 0xff];
+		if (irq->dest_id >= ARRAY_SIZE(map->phys_map))
+			goto out;
+
+		dst = &map->phys_map[irq->dest_id];
 	} else {
 		u32 mda = irq->dest_id << (32 - map->ldr_bits);
 
-- 
2.1.0

[PATCH 3/4] KVM: x86: allow 256 logical x2APICs again

[Radim Krčmář]

While fixing an x2apic bug,
 17d68b7 KVM: x86: fix guest-initiated crash with x2apic (CVE-2013-6376)
we've made only one cluster available.  This means that the amount of
logically addressible x2APICs was reduced to 16 and VCPUs kept
overwriting themselves in that region, so even the first cluster wasn't
set up correctly.

This patch extends x2APIC support back to the logical_map's limit, and
keeps the CVE fixed as messages for non-present APICs are dropped.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/lapic.c | 11 ++++++-----
 arch/x86/kvm/lapic.h |  2 --
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 049d30f..f6e3369 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -132,8 +132,6 @@ static inline int kvm_apic_id(struct kvm_lapic *apic)
 	return (kvm_apic_get_reg(apic, APIC_ID) >> 24) & 0xff;
 }
 
-#define KVM_X2APIC_CID_BITS 0
-
 static void recalculate_apic_map(struct kvm *kvm)
 {
 	struct kvm_apic_map *new, *old = NULL;
@@ -163,8 +161,7 @@ static void recalculate_apic_map(struct kvm *kvm)
 		if (apic_x2apic_mode(apic)) {
 			new->ldr_bits = 32;
 			new->cid_shift = 16;
-			new->cid_mask = (1 << KVM_X2APIC_CID_BITS) - 1;
-			new->lid_mask = 0xffff;
+			new->cid_mask = new->lid_mask = 0xffff;
 			new->broadcast = X2APIC_BROADCAST;
 		} else if (kvm_apic_get_reg(apic, APIC_LDR)) {
 			if (kvm_apic_get_reg(apic, APIC_DFR) ==
@@ -697,8 +694,12 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 		dst = &map->phys_map[irq->dest_id];
 	} else {
 		u32 mda = irq->dest_id << (32 - map->ldr_bits);
+		u16 cid = apic_cluster_id(map, mda);
 
-		dst = map->logical_map[apic_cluster_id(map, mda)];
+		if (cid >= ARRAY_SIZE(map->logical_map))
+			goto out;
+
+		dst = map->logical_map[cid];
 
 		bitmap = apic_logical_id(map, mda);
 
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index d4365f2..c674fce 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -154,8 +154,6 @@ static inline u16 apic_cluster_id(struct kvm_apic_map *map, u32 ldr)
 	ldr >>= 32 - map->ldr_bits;
 	cid = (ldr >> map->cid_shift) & map->cid_mask;
 
-	BUG_ON(cid >= ARRAY_SIZE(map->logical_map));
-
 	return cid;
 }
 
-- 
2.1.0

Re: [PATCH 3/4] KVM: x86: allow 256 logical x2APICs again

[Nadav Amit]

Radim Krčmář <rkrcmar@redhat.com> wrote:

> While fixing an x2apic bug,
> 17d68b7 KVM: x86: fix guest-initiated crash with x2apic (CVE-2013-6376)
> we've made only one cluster available.  This means that the amount of
> logically addressible x2APICs was reduced to 16 and VCPUs kept
> overwriting themselves in that region, so even the first cluster wasn't
> set up correctly.
> 
> This patch extends x2APIC support back to the logical_map's limit, and
> keeps the CVE fixed as messages for non-present APICs are dropped.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
> arch/x86/kvm/lapic.c | 11 ++++++-----
> arch/x86/kvm/lapic.h |  2 --
> 2 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 049d30f..f6e3369 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -132,8 +132,6 @@ static inline int kvm_apic_id(struct kvm_lapic *apic)
> 	return (kvm_apic_get_reg(apic, APIC_ID) >> 24) & 0xff;
> }
> 
> -#define KVM_X2APIC_CID_BITS 0
> -
> static void recalculate_apic_map(struct kvm *kvm)
> {
> 	struct kvm_apic_map *new, *old = NULL;
> @@ -163,8 +161,7 @@ static void recalculate_apic_map(struct kvm *kvm)
> 		if (apic_x2apic_mode(apic)) {
> 			new->ldr_bits = 32;
> 			new->cid_shift = 16;
> -			new->cid_mask = (1 << KVM_X2APIC_CID_BITS) - 1;
> -			new->lid_mask = 0xffff;
> +			new->cid_mask = new->lid_mask = 0xffff;
You set cid_mask to 0xffff, while there are only 16 clusters. I think it is
risky (if you twist my hand would come with a scenario). Yet, why not to set
cid_mask to (ARRAY_SIZE(map->logical_map) - 1) ?


Nadav

Re: [PATCH 3/4] KVM: x86: allow 256 logical x2APICs again

[Radim Krčmář]

2014-11-27 21:53+0200, Nadav Amit:
> Radim Krčmář <rkrcmar@redhat.com> wrote:
> > -			new->cid_mask = (1 << KVM_X2APIC_CID_BITS) - 1;
> > -			new->lid_mask = 0xffff;
> > +			new->cid_mask = new->lid_mask = 0xffff;
> You set cid_mask to 0xffff, while there are only 16 clusters. I think it is
> risky (if you twist my hand would come with a scenario).

Let's see :) APIC id is 8 bit, and we compute cluster part of LDR by
taking four upper bits, so 16 is enough.

It isn't the safest programming practice, but we already fail to check
physical_map bounds and any boost to maximal APIC ID is going to require
a rewrite, thus I didn't bother to do it ...

All uses should be covered with the following hunk, I will add it to v2
after all reviews,

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 6c2b8a5..30e4cc1 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 = apic_cluster_id(new, ldr);
 		lid = apic_logical_id(new, ldr);
 
-		if (lid)
+		if (lid && cid < ARRAY_SIZE(map->logical_map))
 			new->logical_map[cid][ffs(lid) - 1] = apic;
 	}
 out:


>                                                          Yet, why not to set
> cid_mask to (ARRAY_SIZE(map->logical_map) - 1) ?

We would incorrectly deliver messages intended for high clusters,
it has to be 0xffff.

Re: [PATCH 3/4] KVM: x86: allow 256 logical x2APICs again

[Nadav Amit]

Radim Krčmář <rkrcmar@redhat.com> wrote:

> 2014-11-27 21:53+0200, Nadav Amit:
>> Radim Krčmář <rkrcmar@redhat.com> wrote:
>>> -			new->cid_mask = (1 << KVM_X2APIC_CID_BITS) - 1;
>>> -			new->lid_mask = 0xffff;
>>> +			new->cid_mask = new->lid_mask = 0xffff;
>> You set cid_mask to 0xffff, while there are only 16 clusters. I think it is
>> risky (if you twist my hand would come with a scenario).
> 
> Let's see :) APIC id is 8 bit, and we compute cluster part of LDR by
> taking four upper bits, so 16 is enough.
To clarify my concern - I am worried that some of the CPUs are still in
xAPIC mode with LDR that does not follow x2APIC LDR scheme.

> It isn't the safest programming practice, but we already fail to check
> physical_map bounds and any boost to maximal APIC ID is going to require
> a rewrite, thus I didn't bother to do it ...
> 
> All uses should be covered with the following hunk, I will add it to v2
> after all reviews,
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 6c2b8a5..30e4cc1 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 = apic_cluster_id(new, ldr);
> 		lid = apic_logical_id(new, ldr);
> 
> -		if (lid)
> +		if (lid && cid < ARRAY_SIZE(map->logical_map))
> 			new->logical_map[cid][ffs(lid) - 1] = apic;
> 	}
> out:
> 
> 
>>                                                         Yet, why not to set
>> cid_mask to (ARRAY_SIZE(map->logical_map) - 1) ?
> 
> We would incorrectly deliver messages intended for high clusters,
> it has to be 0xffff.
>From the SDM, I am not sure you are correct, but your solution is fine.

Nadav

Re: [PATCH 3/4] KVM: x86: allow 256 logical x2APICs again

[Radim Krčmář]

2014-11-27 22:39+0200, Nadav Amit:
> Radim Krčmář <rkrcmar@redhat.com> wrote:
> 
> > 2014-11-27 21:53+0200, Nadav Amit:
> >> Radim Krčmář <rkrcmar@redhat.com> wrote:
> >>> -			new->cid_mask = (1 << KVM_X2APIC_CID_BITS) - 1;
> >>> -			new->lid_mask = 0xffff;
> >>> +			new->cid_mask = new->lid_mask = 0xffff;
> >> You set cid_mask to 0xffff, while there are only 16 clusters. I think it is
> >> risky (if you twist my hand would come with a scenario).
> > 
> > Let's see :) APIC id is 8 bit, and we compute cluster part of LDR by
> > taking four upper bits, so 16 is enough.
> To clarify my concern - I am worried that some of the CPUs are still in
> xAPIC mode with LDR that does not follow x2APIC LDR scheme.

xAPIC has only the highest LDR byte nonzero, so it won't pass the lid
check.  (Logical xAPIC doesn't work with x2APIC in our implementation.)

I agree, it is really obsucure and I should have put it in v1.

> > We would incorrectly deliver messages intended for high clusters,
> > it has to be 0xffff.
> From the SDM, I am not sure you are correct, but your solution is fine.

We'd have to change the code in irq_deliver...fast to account for that
change, which I think is uglier than checking here.
(If the message is for 0xf0000001 and 0x00000001 accepts it, SDM doesn't
 approve.)

[PATCH 4/4] KVM: x86: don't retry hopeless APIC delivery

[Radim Krčmář]

False from kvm_irq_delivery_to_apic_fast() means that we don't handle it
in the fast path, but we still return false in cases that were perfectly
handled, fix that.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/lapic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index f6e3369..6c2b8a5 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -687,6 +687,8 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 	if (irq->dest_id == map->broadcast)
 		goto out;
 
+	ret = true;
+
 	if (irq->dest_mode == 0) { /* physical mode */
 		if (irq->dest_id >= ARRAY_SIZE(map->phys_map))
 			goto out;
@@ -725,8 +727,6 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 			*r = 0;
 		*r += kvm_apic_set_irq(dst[i]->vcpu, irq, dest_map);
 	}
-
-	ret = true;
 out:
 	rcu_read_unlock();
 	return ret;
-- 
2.1.0

[PATCH 5/4] KVM: x86: check bounds of APIC maps

[Radim Krčmář]

They can't be violated now, but we think against the infinite thing.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 I realized it could make a separate patch as well,
 which might be more convenient.

 arch/x86/kvm/lapic.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 6c2b8a5..ae019f6 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -190,15 +190,16 @@ static void recalculate_apic_map(struct kvm *kvm)
 	kvm_for_each_vcpu(i, vcpu, kvm) {
 		struct kvm_lapic *apic = vcpu->arch.apic;
 		u16 cid, lid;
-		u32 ldr;
-
-		new->phys_map[kvm_apic_id(apic)] = apic;
+		u32 ldr, aid;
 
+		aid = kvm_apic_id(apic);
 		ldr = kvm_apic_get_reg(apic, APIC_LDR);
 		cid = apic_cluster_id(new, ldr);
 		lid = apic_logical_id(new, ldr);
 
-		if (lid)
+		if (aid < ARRAY_SIZE(new->phys_map))
+			new->phys_map[aid] = apic;
+		if (lid && cid < ARRAY_SIZE(new->logical_map))
 			new->logical_map[cid][ffs(lid) - 1] = apic;
 	}
 out:
-- 
2.1.0

Re: [PATCH 0/4] KVM: x86: APIC fixes

[Paolo Bonzini]

On 27/11/2014 20:03, Radim Krčmář wrote:
> The interesting one is [3/4], which improves upon a previous CVE fix;
> we also handle logical destination wrapping in it, so [2/4] does the
> same for physical;  and to make it nicer, [1/4] removes a condition.
> [4/4] makes our fast path return true when the message was handled.
> 
> Radim Krčmář (4):
>   KVM: x86: deliver phys lowest-prio
>   KVM: x86: fix APIC physical destination wrapping
>   KVM: x86: allow 256 logical x2APICs again
>   KVM: x86: don't retry hopeless APIC delivery
> 
>  arch/x86/kvm/lapic.c | 20 +++++++++++---------
>  arch/x86/kvm/lapic.h |  2 --
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 

So the order should be 1/2/5/3/4, right?

Paolo

Re: [PATCH 0/4] KVM: x86: APIC fixes

[Radim Krčmář]

2014-12-01 17:22+0100, Paolo Bonzini:
> On 27/11/2014 20:03, Radim Krčmář wrote:
> > The interesting one is [3/4], which improves upon a previous CVE fix;
> > we also handle logical destination wrapping in it, so [2/4] does the
> > same for physical;  and to make it nicer, [1/4] removes a condition.
> > [4/4] makes our fast path return true when the message was handled.
> > 
> > Radim Krčmář (4):
> >   KVM: x86: deliver phys lowest-prio
> >   KVM: x86: fix APIC physical destination wrapping
> >   KVM: x86: allow 256 logical x2APICs again
> >   KVM: x86: don't retry hopeless APIC delivery
> > 
> >  arch/x86/kvm/lapic.c | 20 +++++++++++---------
> >  arch/x86/kvm/lapic.h |  2 --
> >  2 files changed, 11 insertions(+), 11 deletions(-)
> > 
> 
> So the order should be 1/2/5/3/4, right?

It would be safer, thank you.

(And when I look at it now, [4/4] would be better as 1st.)

Re: [PATCH 0/4] KVM: x86: APIC fixes

[Paolo Bonzini]

On 01/12/2014 18:55, Radim Krčmář wrote:
> 2014-12-01 17:22+0100, Paolo Bonzini:
>> On 27/11/2014 20:03, Radim Krčmář wrote:
>>> The interesting one is [3/4], which improves upon a previous CVE fix;
>>> we also handle logical destination wrapping in it, so [2/4] does the
>>> same for physical;  and to make it nicer, [1/4] removes a condition.
>>> [4/4] makes our fast path return true when the message was handled.
>>>
>>> Radim Krčmář (4):
>>>   KVM: x86: deliver phys lowest-prio
>>>   KVM: x86: fix APIC physical destination wrapping
>>>   KVM: x86: allow 256 logical x2APICs again
>>>   KVM: x86: don't retry hopeless APIC delivery
>>>
>>>  arch/x86/kvm/lapic.c | 20 +++++++++++---------
>>>  arch/x86/kvm/lapic.h |  2 --
>>>  2 files changed, 11 insertions(+), 11 deletions(-)
>>>
>>
>> So the order should be 1/2/5/3/4, right?
> 
> It would be safer, thank you.
> 
> (And when I look at it now, [4/4] would be better as 1st.)

Ok, applying 4/1/2/5/3.

Paolo