Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access

Radim Krcmar
2018-08-29T14:36:55Z

2018-08-29 15:55+0200, Radim Krcmar:
> 2018-08-29 13:43+0300, Liran Alon:
> > Why is “min” defined as “int” instead of “unsigned int”?
> > It represents the lowest APIC ID in bitmap so it can’t be negative…
>
> Right,
>
> I think the code would look better as something like (untested):
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 0cefba28c864..24fc84eb97d2 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -548,7 +548,7 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
> }
>
> int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> - unsigned long ipi_bitmap_high, int min,
> + unsigned long ipi_bitmap_high, u32 min,
> unsigned long icr, int op_64_bit)
> {
> int i;
> @@ -557,6 +557,7 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> struct kvm_lapic_irq irq = {0};
> int cluster_size = op_64_bit ? 64 : 32;
> int count = 0;
> + unsigned long ipi_bitmap[2] = {ipi_bitmap_low, ipi_bitmap_high};

The patch is wrong, I missed the 32/64 bit cluster size.

It's salvageable with something like

if (op_64_bit) {
ipi_bitmap[0] = ipi_bitmap_low;
ipi_bitmap[1] = ipi_bitmap_high;
ipi_bitmap_size = 128;
} else {
ipi_bitmap[0] = (u32)ipi_bitmap_low | ipi_bitmap_high << 32;
ipi_bitmap_size = 64;
}

>
> irq.vector = icr & APIC_VECTOR_MASK;
> irq.delivery_mode = icr & APIC_MODE_MASK;
> @@ -571,16 +572,14 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> rcu_read_lock();
> map = rcu_dereference(kvm->arch.apic_map);
>
> - /* Bits above cluster_size are masked in the caller. */
> - for_each_set_bit(i, &ipi_bitmap_low, BITS_PER_LONG) {
> - vcpu = map->phys_map[min + i]->vcpu;
> - count += kvm_apic_set_irq(vcpu, &irq, NULL);
> - }
> + if (min <= map->max_apic_id) {
> + size_t ipi_bitmap_size = MIN(sizeof(ipi_bitmap) * 8,
> + map->max_apic_id - min + 1);
> - min += cluster_size;
> - for_each_set_bit(i, &ipi_bitmap_high, BITS_PER_LONG) {
> - vcpu = map->phys_map[min + i]->vcpu;
> - count += kvm_apic_set_irq(vcpu, &irq, NULL);
> + for_each_set_bit(i, ipi_bitmap, ipi_bitmap_size) {

and

... MIN(ipi_bitmap_size, map->max_apic_id - min + 1) ...

Not good, but could still be nicer than the alternatives.

> + vcpu = map->phys_map[min + i]->vcpu;
> + count += kvm_apic_set_irq(vcpu, &irq, NULL);
> + }
> }
>
> rcu_read_unlock();