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
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ář <[email protected]>
---
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
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ář <[email protected]>
---
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
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ář <[email protected]>
---
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
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ář <[email protected]>
---
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
Radim Krčmář <[email protected]> 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ář <[email protected]>
> ---
> 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-
2014-11-27 21:53+0200, Nadav Amit:
> Radim Krčmář <[email protected]> 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.
Radim Krčmář <[email protected]> wrote:
> 2014-11-27 21:53+0200, Nadav Amit:
>> Radim Krčmář <[email protected]> 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
2014-11-27 22:39+0200, Nadav Amit:
> Radim Krčmář <[email protected]> wrote:
>
> > 2014-11-27 21:53+0200, Nadav Amit:
> >> Radim Krčmář <[email protected]> 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.)
They can't be violated now, but we think against the infinite thing.
Signed-off-by: Radim Krčmář <[email protected]>
---
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
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
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.)
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