lkml.org 
[lkml]   [2018]   [Aug]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access
    From
    Date


    > On 29 Aug 2018, at 8:52, Wanpeng Li <kernellwp@gmail.com> wrote:
    >
    > From: Wanpeng Li <wanpengli@tencent.com>
    >
    > Dan Carpenter reported that the untrusted data returns from kvm_register_read()
    > results in the following static checker warning:
    > arch/x86/kvm/lapic.c:576 kvm_pv_send_ipi()
    > error: buffer underflow 'map->phys_map' 's32min-s32max'
    >
    > 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.
    >
    > This patch fixes it by adding a check to kvm_pv_send_ipi() against map->max_apic_id
    > and also checking whether or not map->phys_map[min + i] is NULL since the max_apic_id
    > is set according to the max apic id, however, some phys_map maybe NULL when apic id
    > is sparse, in addition, kvm also unconditionally set max_apic_id to 255 to reserve
    > enough space for any xAPIC ID.
    >
    > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
    > Cc: Paolo Bonzini <pbonzini@redhat.com>
    > Cc: Radim Krčmář <rkrcmar@redhat.com>
    > Cc: Liran Alon <liran.alon@oracle.com>
    > Cc: Dan Carpenter <dan.carpenter@oracle.com>
    > Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
    > ---
    > arch/x86/kvm/lapic.c | 17 +++++++++++++----
    > 1 file changed, 13 insertions(+), 4 deletions(-)
    >
    > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
    > index 0cefba2..86e933c 100644
    > --- a/arch/x86/kvm/lapic.c
    > +++ b/arch/x86/kvm/lapic.c
    > @@ -571,18 +571,27 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
    > rcu_read_lock();
    > map = rcu_dereference(kvm->arch.apic_map);
    >
    > + if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_low)) < min))
    > + goto out;

    I personally think “if ((min + __fls(ipi_bitmap_low)) > map->max_apic_id)” is more readable.
    But that’s just a matter of taste :)

    > /* 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 (map->phys_map[min + i]) {
    > + vcpu = map->phys_map[min + i]->vcpu;
    > + count += kvm_apic_set_irq(vcpu, &irq, NULL);
    > + }
    > }
    >
    > min += cluster_size;
    > + if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_high)) < min))
    > + goto out;
    > 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);
    > + if (map->phys_map[min + i]) {
    > + vcpu = map->phys_map[min + i]->vcpu;
    > + count += kvm_apic_set_irq(vcpu, &irq, NULL);
    > + }
    > }
    >
    > +out:
    > rcu_read_unlock();
    > return count;
    > }
    > --
    > 2.7.4
    >

    Reviewed-By: Liran Alon <liran.alon@oracle.com>


    \
     
     \ /
      Last update: 2018-08-29 11:05    [W:3.493 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site