[Devel] [PATCH RHEL7 COMMIT] KVM: LAPIC: Fix pv ipis out-of-bounds access

[Vasily Averin]

The commit is pushed to "branch-rh7-3.10.0-1127.18.2.vz7.163.x-ovz" and will appear at https://src.openvz.org/scm/ovz/vzkernel.git
after rh7-3.10.0-1127.18.2.vz7.163.27
------>
commit 69be0b6e4ce2dee6e42bf89a7497d1fef2c4e2d0
Author: Wanpeng Li <wanpengli at tencent.com>
Date:   Tue Sep 22 10:58:11 2020 +0300

    KVM: LAPIC: Fix pv ipis out-of-bounds access
    
    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,
    ignoring destinations that are not present and delivering the rest. We also check
    whether or not map->phys_map[min + i] is NULL since the max_apic_id is set to the
    max apic id, some phys_map maybe NULL when apic id is sparse, especially kvm
    unconditionally set max_apic_id to 255 to reserve enough space for any xAPIC ID.
    
    Reported-by: Dan Carpenter <dan.carpenter at oracle.com>
    Reviewed-by: Liran Alon <liran.alon at oracle.com>
    Cc: Paolo Bonzini <pbonzini at redhat.com>
    Cc: Radim Krčmář <rkrcmar at redhat.com>
    Cc: Liran Alon <liran.alon at oracle.com>
    Cc: Dan Carpenter <dan.carpenter at oracle.com>
    Signed-off-by: Wanpeng Li <wanpengli at tencent.com>
    [Add second "if (min > map->max_apic_id)" to complete the fix. -Radim]
    Signed-off-by: Radim Krčmář <rkrcmar at redhat.com>
    
    (cherry picked from commit bdf7ffc89922a52a4f08a12f7421ea24bb7626a0)
    https://jira.sw.ru/browse/PSBM-107931
    
    Signed-off-by: Valeriy Vdovin <valeriy.vdovin at virtuozzo.com>
---
 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/lapic.c            | 27 ++++++++++++++++++++-------
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 50817dc3..e9ee080 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1434,7 +1434,7 @@ void kvm_arch_mmu_notifier_invalidate_page(struct kvm *kvm,
 
 u64 kvm_get_arch_capabilities(void);
 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);
 
 void kvm_define_shared_msr(unsigned index, u32 msr);
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 1487fe2..740be89 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -543,7 +543,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;
@@ -566,18 +566,31 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
 	rcu_read_lock();
 	map = rcu_dereference(kvm->arch.apic_map);
 
+	if (min > map->max_apic_id)
+		goto out;
 	/* 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);
+	for_each_set_bit(i, &ipi_bitmap_low,
+		min((u32)BITS_PER_LONG, (map->max_apic_id - min + 1))) {
+		if (map->phys_map[min + i]) {
+			vcpu = map->phys_map[min + i]->vcpu;
+			count += kvm_apic_set_irq(vcpu, &irq, NULL);
+		}
 	}
 
 	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);
+
+	if (min > map->max_apic_id)
+		goto out;
+
+	for_each_set_bit(i, &ipi_bitmap_high,
+		min((u32)BITS_PER_LONG, (map->max_apic_id - min + 1))) {
+		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;
 }