From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754760AbcGKNso (ORCPT ); Mon, 11 Jul 2016 09:48:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59737 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751565AbcGKNsm (ORCPT ); Mon, 11 Jul 2016 09:48:42 -0400 Date: Mon, 11 Jul 2016 15:48:38 +0200 From: Radim =?utf-8?B?S3LEjW3DocWZ?= To: Yang Zhang Cc: Paolo Bonzini , linux-kernel@vger.kernel.org, kvm@vger.kernel.org, "Lan, Tianyu" , Igor Mammedov , Jan Kiszka , Peter Xu Subject: Re: [PATCH v2 04/13] KVM: x86: dynamic kvm_apic_map Message-ID: <20160711134837.GD21976@potion> References: <20160707171550.14675-1-rkrcmar@redhat.com> <20160707171550.14675-5-rkrcmar@redhat.com> <963b542a-1111-db83-8338-c32d44f98874@gmail.com> <3a5d86b6-9f1a-a6cf-8af4-ef6bf3936996@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3a5d86b6-9f1a-a6cf-8af4-ef6bf3936996@gmail.com> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Mon, 11 Jul 2016 13:48:41 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 2016-07-11 18:14+0800, Yang Zhang: > On 2016/7/11 15:43, Paolo Bonzini wrote: >> On 11/07/2016 08:07, Yang Zhang wrote: >> > > >> > > mutex_lock(&kvm->arch.apic_map_lock); >> > > >> > > + kvm_for_each_vcpu(i, vcpu, kvm) >> > > + if (kvm_apic_present(vcpu)) >> > > + max_id = max(max_id, kvm_apic_id(vcpu->arch.apic)); >> > > + >> > > + new = kzalloc(sizeof(struct kvm_apic_map) + >> > > + sizeof(struct kvm_lapic *) * (max_id + 1), >> > > GFP_KERNEL); >> > > + >> > >> > I think this may cause the host runs out of memory if a malicious guest >> > did follow thing: >> > 1. vcpu a is doing apic map recalculation. >> > 2. vcpu b write the apic id with 0xff >> > 3. then vcpu b enable the x2apic: in kvm_lapic_set_base(), we will set >> > apic_base to new value before reset the apic id. >> > 4. vcpu a may see the x2apic enabled in vcpu b plus an old apic >> > id(0xff), and max_id will become (0xff >> 24). Indeed, thanks. The guest doesn't even have to be malicious ... >> The bug is not really here but in patch 6---but you're right nevertheless! Yes. >> I guess the easiest solution is to replace kvm_apic_id with a field in >> struct kvm_lapic, which is already shifted right by 24 in xAPIC mode. (I guess the fewest LOC is to look at vcpu->vcpu_id, which is equal to x2apic id. xapic id cannot be greater than 255 and all of those are covered by the initial value of max_id.) > Or we can just simply put the assignment of apic_base to the end. Yes, this would work, I'd also remove recalculates from kvm_apic_set_*apic_id() and add a compiler barrier with comment for good measure, even though set_virtual_x2apic_mode() serves as one. (What makes a bit wary is that it doesn't avoid the same problem if we changed KVM to reset apic id to xapic id first when disabling apic.) Races in recalculation and APIC ID changes also lead to invalid physical maps, which haven't been taken care of properly ... Having apic id stored in big endian, or "0-7,8-31" format, would be safest. I wanted to change the apic map to do incremental updates with with respect to the APIC that has changed, instead of being completely recomputed, so maybe the time is now. :)