From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH v3 7/8] KVM: MMU: Validate all gptes during fetch, not just those used for new pages Date: Tue, 13 Jul 2010 07:18:02 +0300 Message-ID: <4C3BE8FA.8050607@redhat.com> References: <1278951351-6300-1-git-send-email-avi@redhat.com> <1278951351-6300-8-git-send-email-avi@redhat.com> <4C3BC68B.5050205@cn.fujitsu.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , kvm@vger.kernel.org To: Xiao Guangrong Return-path: Received: from mx1.redhat.com ([209.132.183.28]:34656 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750737Ab0GMESN (ORCPT ); Tue, 13 Jul 2010 00:18:13 -0400 In-Reply-To: <4C3BC68B.5050205@cn.fujitsu.com> Sender: kvm-owner@vger.kernel.org List-ID: On 07/13/2010 04:51 AM, Xiao Guangrong wrote: > > Avi Kivity wrote: > >> Currently, when we fetch an spte, we only verify that gptes match those that >> the walker saw if we build new shadow pages for them. >> >> However, this misses the following race: >> >> vcpu1 vcpu2 >> >> walk >> change gpte >> walk >> instantiate sp >> >> fetch existing sp >> >> Fix by validating every gpte, regardless of whether it is used for building >> a new sp or not. >> >> Signed-off-by: Avi Kivity >> --- >> arch/x86/kvm/paging_tmpl.h | 44 +++++++++++++++++++++++++++++++------------- >> 1 files changed, 31 insertions(+), 13 deletions(-) >> >> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h >> index 441f51c..89b2dab 100644 >> --- a/arch/x86/kvm/paging_tmpl.h >> +++ b/arch/x86/kvm/paging_tmpl.h >> @@ -310,7 +310,8 @@ static bool FNAME(validate_indirect_spte)(struct kvm_vcpu *vcpu, >> gw->pte_gpa[level - 1], >> &curr_pte, sizeof(curr_pte)); >> if (r || curr_pte != gw->ptes[level - 1]) { >> - kvm_mmu_put_page(sp, sptep); >> + if (sp) >> + kvm_mmu_put_page(sp, sptep); >> return false; >> } >> return true; >> @@ -325,10 +326,11 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr, >> int *ptwrite, pfn_t pfn) >> { >> unsigned access = gw->pt_access; >> - struct kvm_mmu_page *sp; >> + struct kvm_mmu_page *uninitialized_var(sp); >> u64 *sptep = NULL; >> int uninitialized_var(level); >> bool dirty = is_dirty_gpte(gw->ptes[gw->level - 1]); >> + int top_level; >> unsigned direct_access; >> struct kvm_shadow_walk_iterator iterator; >> >> @@ -339,34 +341,46 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr, >> if (!dirty) >> direct_access&= ~ACC_WRITE_MASK; >> >> + top_level = vcpu->arch.mmu.root_level; >> + if (top_level == PT32E_ROOT_LEVEL) >> + top_level = PT32_ROOT_LEVEL; >> + /* >> + * Verify that the top-level gpte is still there. Since the page >> + * is a root page, it is either write protected (and cannot be >> + * changed from now on) or it is invalid (in which case, we don't >> + * really care if it changes underneath us after this point). >> + */ >> + if (!FNAME(validate_indirect_spte)(vcpu, NULL, NULL, gw, top_level)) >> + goto out_error; >> + >> for (shadow_walk_init(&iterator, vcpu, addr); >> shadow_walk_okay(&iterator)&& iterator.level> gw->level; >> shadow_walk_next(&iterator)) { >> gfn_t table_gfn; >> + bool new_page = false; >> >> level = iterator.level; >> sptep = iterator.sptep; >> >> drop_large_spte(vcpu, sptep); >> >> - if (is_shadow_present_pte(*sptep)) >> - continue; >> - >> - table_gfn = gw->table_gfn[level - 2]; >> - sp = kvm_mmu_get_page(vcpu, table_gfn, addr, level-1, >> - false, access, sptep); >> + if (!is_shadow_present_pte(*sptep)) { >> + table_gfn = gw->table_gfn[level - 2]; >> + sp = kvm_mmu_get_page(vcpu, table_gfn, addr, level-1, >> + false, access, sptep); >> + new_page = true; >> + } >> >> /* >> * Verify that the gpte in the page we've just write >> * protected is still there. >> */ >> if (!FNAME(validate_indirect_spte)(vcpu, sptep, sp, >> - gw, level - 1)) { >> - kvm_release_pfn_clean(pfn); >> - return NULL; >> - } >> + gw, level - 1)) >> + goto out_error; >> >> > It missed the last mapping check? i only see validate_indirect_spte in > 'level> gw->level' loop. > But we check 'level - 1' here, so the final level is included. It is the top level that is not checked in the loop (we check it separately, above). -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.