From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932922AbdELJPi (ORCPT ); Fri, 12 May 2017 05:15:38 -0400 Received: from foss.arm.com ([217.140.101.70]:57396 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753208AbdELJPf (ORCPT ); Fri, 12 May 2017 05:15:35 -0400 Subject: Re: [PATCH 2/2] kvm: arm/arm64: Fix use after free of stage2 page table To: Suzuki K Poulose , christoffer.dall@linaro.org References: <1493821072-29713-1-git-send-email-suzuki.poulose@arm.com> <1493821072-29713-3-git-send-email-suzuki.poulose@arm.com> Cc: agraf@suse.de, andreyknvl@google.com, mark.rutland@arm.com, pbonzini@redhat.com, rkrcmar@redhat.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org From: Marc Zyngier Organization: ARM Ltd Message-ID: <23e9374e-90d8-2240-58bc-15ff77d2d4ab@arm.com> Date: Fri, 12 May 2017 10:15:32 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <1493821072-29713-3-git-send-email-suzuki.poulose@arm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03/05/17 15:17, Suzuki K Poulose wrote: > We yield the kvm->mmu_lock occassionaly while performing an operation occasionally > (e.g, unmap or permission changes) on a large area of stage2 mappings. > However this could possibly cause another thread to clear and free up > the stage2 page tables while we were waiting for regaining the lock and > thus the original thread could end up in accessing memory that was > freed. This patch fixes the problem by making sure that the stage2 > pagetable is still valid after we regain the lock. The fact that > mmu_notifer->release() could be called twice (via __mmu_notifier_release > and mmu_notifier_unregsister) enhances the possibility of hitting unregister > this race where there are two threads trying to unmap the entire guest > shadow pages. > > While at it, cleanup the redudant checks around cond_resched_lock in redundant > stage2_wp_range(), as cond_resched_lock already does the same checks. > > Cc: Mark Rutland > Cc: Radim Krčmář > Cc: andreyknvl@google.com > Cc: Christoffer Dall > Cc: Marc Zyngier > Cc: Paolo Bonzini > Signed-off-by: Suzuki K Poulose > --- > arch/arm/kvm/mmu.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c > index 909a1a7..5b3e0db 100644 > --- a/arch/arm/kvm/mmu.c > +++ b/arch/arm/kvm/mmu.c > @@ -301,9 +301,14 @@ static void unmap_stage2_range(struct kvm *kvm, phys_addr_t start, u64 size) > /* > * If the range is too large, release the kvm->mmu_lock > * to prevent starvation and lockup detector warnings. > + * Make sure the page table is still active when we regain > + * the lock. > */ > - if (next != end) > + if (next != end) { > cond_resched_lock(&kvm->mmu_lock); > + if (!READ_ONCE(kvm->arch.pgd)) > + break; > + } > } while (pgd++, addr = next, addr != end); > } > > @@ -1170,11 +1175,13 @@ static void stage2_wp_range(struct kvm *kvm, phys_addr_t addr, phys_addr_t end) > * large. Otherwise, we may see kernel panics with > * CONFIG_DETECT_HUNG_TASK, CONFIG_LOCKUP_DETECTOR, > * CONFIG_LOCKDEP. Additionally, holding the lock too long > - * will also starve other vCPUs. > + * will also starve other vCPUs. We have to also make sure > + * that the page tables are not freed while we released > + * the lock. > */ > - if (need_resched() || spin_needbreak(&kvm->mmu_lock)) > - cond_resched_lock(&kvm->mmu_lock); > - > + cond_resched_lock(&kvm->mmu_lock); > + if (!READ_ONCE(kvm->arch.pgd)) > + break; > next = stage2_pgd_addr_end(addr, end); > if (stage2_pgd_present(*pgd)) > stage2_wp_puds(pgd, addr, next); > Reviewed-by: Marc Zyngier Thanks, M. -- Jazz is not dead. It just smells funny...