From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PATCH v2] KVM: LAPIC: Fix pv ipis out-of-bounds access Date: Thu, 30 Aug 2018 13:01:21 +0200 Message-ID: <20180830110121.GB19438@flask> References: <1535594610-9095-1-git-send-email-wanpengli@tencent.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, Paolo Bonzini , Liran Alon , Dan Carpenter To: Wanpeng Li Return-path: Content-Disposition: inline In-Reply-To: <1535594610-9095-1-git-send-email-wanpengli@tencent.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org 2018-08-30 10:03+0800, Wanpeng Li: > From: Wanpeng Li > > 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 > Reviewed-by: Liran Alon > Cc: Paolo Bonzini > Cc: Radim Krčmář > Cc: Liran Alon > Cc: Dan Carpenter > Signed-off-by: Wanpeng Li > --- > v1 -> v2: > * add min > map->max_apic_id check > * change min to u32 > * add min((u32)BITS_PER_LONG, (map->max_apic_id - min + 1)) > > diff --git 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; > @@ -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 (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; We need a second if (min > map->max_apic_id) goto out; here. I will add it while applying if there are no other change requests. > - 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_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; > } > -- > 2.7.4 >