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

[Avi Kivity <avi@redhat.com>]

On 07/12/2010 10:16 PM, Marcelo Tosatti wrote:
> 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?
>    

It can, and then sp and sptep are inconsistent.  Good catch.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.