> On 28 Aug 2018, at 16:28, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> Hello Wanpeng Li,
>
> The patch 4180bf1b655a: "KVM: X86: Implement "send IPI" hypercall"
> from Jul 23, 2018, leads to the following static checker warning:
>
> arch/x86/kvm/lapic.c:576 kvm_pv_send_ipi()
> error: buffer underflow 'map->phys_map' 's32min-s32max'
>
> arch/x86/kvm/lapic.c
> 550 int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> 551 unsigned long ipi_bitmap_high, int min,
> 552 unsigned long icr, int op_64_bit)
> 553 {
> 554 int i;
> 555 struct kvm_apic_map *map;
> 556 struct kvm_vcpu *vcpu;
> 557 struct kvm_lapic_irq irq = {0};
> 558 int cluster_size = op_64_bit ? 64 : 32;
> 559 int count = 0;
> 560
> 561 irq.vector = icr & APIC_VECTOR_MASK;
> 562 irq.delivery_mode = icr & APIC_MODE_MASK;
> 563 irq.level = (icr & APIC_INT_ASSERT) != 0;
> 564 irq.trig_mode = icr & APIC_INT_LEVELTRIG;
> 565
> 566 if (icr & APIC_DEST_MASK)
> 567 return -KVM_EINVAL;
> 568 if (icr & APIC_SHORT_MASK)
> 569 return -KVM_EINVAL;
> 570
> 571 rcu_read_lock();
> 572 map = rcu_dereference(kvm->arch.apic_map);
> 573
> 574 /* Bits above cluster_size are masked in the caller. */
> 575 for_each_set_bit(i, &ipi_bitmap_low, BITS_PER_LONG) {
> 576 vcpu = map->phys_map[min + i]->vcpu;
> ^^^^^^^
> 577 count += kvm_apic_set_irq(vcpu, &irq, NULL);
> 578 }
> 579
> 580 min += cluster_size;
> 581 for_each_set_bit(i, &ipi_bitmap_high, BITS_PER_LONG) {
> 582 vcpu = map->phys_map[min + i]->vcpu;
> ^^^^^^^
> The truth is that I don't understand KVM very well so I'm just reporting
> these warning in the hope that someone will explain things to me. This
> is called from kvm_emulate_hypercall(). Recently we marked the return
> from kvm_register_read() as untrusted data. Now Smatch thinks that
> "min" is totally untrusted and there are no boundary checks.
>
> Btw the reason Smatch only warns about a buffer underflow instead of a
> buffer overflow is because it doesn't know the size of map->phys_map[].
>
> When can kvm_register_read() be trusted?
>
> 583 count += kvm_apic_set_irq(vcpu, &irq, NULL);
> 584 }
> 585
> 586 rcu_read_unlock();
> 587 return count;
> 588 }
>
> regards,
> dan carpenter
Thanks Dan.
It seems to be a real issue.
KVM guest can easily trigger this by executing the following assembly sequence in Ring0:
mov $10, %rax
mov $0xFFFFFFFF, %rbx
mov $0xFFFFFFFF, %rdx
mov $0, %rsi
vmcall
As this will cause KVM to execute the following code-path:
vmx_handle_exit() -> handle_vmcall() -> kvm_emulate_hypercall() -> kvm_pv_send_ipi()
which will reach out-of-bounds access.
We should add a check to kvm_pv_send_ipi() against map->max_apic_id.
-Liran
