Re: [PATCH 5/5] kvm: x86: mmu: Verify that restored PTE has needed perms in fast page fault

[Junaid Shahid <junaids@google.com>]

Looks good. Thanks.

On Wednesday, January 25, 2017 12:49:06 PM Paolo Bonzini wrote:
> 
> > -		remove_acc_track = is_access_track_spte(spte);
> > -
> > -		/* Verify that the fault can be handled in the fast path */
> > -		if (!remove_acc_track && !remove_write_prot)
> > -			break;
> > +		new_spte = is_access_track_spte(spte)
> > +			   ? restore_acc_track_spte(spte)
> > +			   : spte;
> 
> Just a tiny stylistic change to match the "new_spte |= PT_WRITABLE_MASK"
> in the block below:
> 
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index aa3b0d14c019..2fd7586aad4d 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -3135,9 +3135,10 @@ static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level,
>  			break;
>  		}
>  
> -		new_spte = is_access_track_spte(spte)
> -			   ? restore_acc_track_spte(spte)
> -			   : spte;
> +		new_spte = spte;
> +
> +		if (is_access_track_spte(spte))
> +			new_spte = restore_acc_track_spte(new_spte);
>  
>  		/*
>  		 * Currently, to simplify the code, write-protection can
> 
> Paolo
> 
> >  		/*
> > -		 * Do not fix write-permission on the large spte since we only
> > -		 * dirty the first page into the dirty-bitmap in
> > -		 * fast_pf_fix_direct_spte() that means other pages are missed
> > -		 * if its slot is dirty-logged.
> > -		 *
> > -		 * Instead, we let the slow page fault path create a normal spte
> > -		 * to fix the access.
> > -		 *
> > -		 * See the comments in kvm_arch_commit_memory_region().
> > +		 * Currently, to simplify the code, write-protection can
> > +		 * be removed in the fast path only if the SPTE was
> > +		 * write-protected for dirty-logging or access tracking.
> >  		 */
> > -		if (sp->role.level > PT_PAGE_TABLE_LEVEL && remove_write_prot)
> > +		if ((error_code & PFERR_WRITE_MASK) &&
> > +		    spte_can_locklessly_be_made_writable(spte))
> > +		{
> > +			new_spte |= PT_WRITABLE_MASK;
> > +
> > +			/*
> > +			 * Do not fix write-permission on the large spte since
> > +			 * we only dirty the first page into the dirty-bitmap in
> > +			 * fast_pf_fix_direct_spte() that means other pages are
> > +			 * missed if its slot is dirty-logged.
> > +			 *
> > +			 * Instead, we let the slow page fault path create a
> > +			 * normal spte to fix the access.
> > +			 *
> > +			 * See the comments in kvm_arch_commit_memory_region().
> > +			 */
> > +			if (sp->role.level > PT_PAGE_TABLE_LEVEL)
> > +				break;
> > +		}
> > +
> > +		/* Verify that the fault can be handled in the fast path */
> > +		if (new_spte == spte ||
> > +		    !is_access_allowed(error_code, new_spte))
> >  			break;
> >  
> >  		/*
> > @@ -3166,8 +3175,7 @@ static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level,
> >  		 */
> >  		fault_handled = fast_pf_fix_direct_spte(vcpu, sp,
> >  							iterator.sptep, spte,
> > -							remove_write_prot,
> > -							remove_acc_track);
> > +							new_spte);
> >  		if (fault_handled)
> >  			break;
> >  
> >