Re: [PATCH v3 7/8] KVM: MMU: Validate all gptes during fetch, not just those used for new pages

[Marcelo Tosatti <mtosatti@redhat.com>]

On Mon, Jul 12, 2010 at 07:15:50PM +0300, 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 <avi@redhat.com>
> ---
>  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;

If its not a new page, and validation fails, can't "sp" point to
a shadow page previously instantiated in the loop?