KVM: MMU: bail out pagewalk on kvm_read_guest error

KVM: MMU: bail out pagewalk on kvm_read_guest error

[Marcelo Tosatti]

Exit the guest pagetable walk loop if reading gpte failed. Otherwise its
possible to enter an endless loop processing the previous present pte.

Cc: stable@kernel.org
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 58a0f1e..ede2131 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -150,7 +150,9 @@ walk:
 		walker->table_gfn[walker->level - 1] = table_gfn;
 		walker->pte_gpa[walker->level - 1] = pte_gpa;
 
-		kvm_read_guest(vcpu->kvm, pte_gpa, &pte, sizeof(pte));
+		if (kvm_read_guest(vcpu->kvm, pte_gpa, &pte, sizeof(pte)))
+			goto not_present;
+
 		trace_kvm_mmu_paging_element(pte, walker->level);
 
 		if (!is_present_gpte(pte))

Re: KVM: MMU: bail out pagewalk on kvm_read_guest error

[Avi Kivity]

On 01/14/2010 09:41 PM, Marcelo Tosatti wrote:
> Exit the guest pagetable walk loop if reading gpte failed. Otherwise its
> possible to enter an endless loop processing the previous present pte.
>
> Cc: stable@kernel.org
> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>
> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
> index 58a0f1e..ede2131 100644
> --- a/arch/x86/kvm/paging_tmpl.h
> +++ b/arch/x86/kvm/paging_tmpl.h
> @@ -150,7 +150,9 @@ walk:
>   		walker->table_gfn[walker->level - 1] = table_gfn;
>   		walker->pte_gpa[walker->level - 1] = pte_gpa;
>
> -		kvm_read_guest(vcpu->kvm, pte_gpa,&pte, sizeof(pte));
> +		if (kvm_read_guest(vcpu->kvm, pte_gpa,&pte, sizeof(pte)))
> +			goto not_present;
> +
>    

On real hardware, if you place a pte at non-existing memory, you aren't 
guaranteed to get the present bit clear, so why is this necessary?

We should be able to survive any garbage the pte previously contained.

-- 
error compiling committee.c: too many arguments to function

Re: KVM: MMU: bail out pagewalk on kvm_read_guest error

[Marcelo Tosatti]

On Sun, Jan 17, 2010 at 10:23:37AM +0200, Avi Kivity wrote:
> On 01/14/2010 09:41 PM, Marcelo Tosatti wrote:
> >Exit the guest pagetable walk loop if reading gpte failed. Otherwise its
> >possible to enter an endless loop processing the previous present pte.
> >
> >Cc: stable@kernel.org
> >Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
> >
> >diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
> >index 58a0f1e..ede2131 100644
> >--- a/arch/x86/kvm/paging_tmpl.h
> >+++ b/arch/x86/kvm/paging_tmpl.h
> >@@ -150,7 +150,9 @@ walk:
> >  		walker->table_gfn[walker->level - 1] = table_gfn;
> >  		walker->pte_gpa[walker->level - 1] = pte_gpa;
> >
> >-		kvm_read_guest(vcpu->kvm, pte_gpa,&pte, sizeof(pte));
> >+		if (kvm_read_guest(vcpu->kvm, pte_gpa,&pte, sizeof(pte)))
> >+			goto not_present;
> >+
> 
> On real hardware, if you place a pte at non-existing memory, you
> aren't guaranteed to get the present bit clear, so why is this
> necessary?
> 
> We should be able to survive any garbage the pte previously contained.

The problem is the content of the previous pte is processed (which is
valid), but the cmpxchg fails (see the loop), without level decreasing.

Re: KVM: MMU: bail out pagewalk on kvm_read_guest error

[Avi Kivity]

On 01/18/2010 07:05 PM, Marcelo Tosatti wrote:
> On Sun, Jan 17, 2010 at 10:23:37AM +0200, Avi Kivity wrote:
>    
>> On 01/14/2010 09:41 PM, Marcelo Tosatti wrote:
>>      
>>> Exit the guest pagetable walk loop if reading gpte failed. Otherwise its
>>> possible to enter an endless loop processing the previous present pte.
>>>
>>> Cc: stable@kernel.org
>>> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>>>
>>> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
>>> index 58a0f1e..ede2131 100644
>>> --- a/arch/x86/kvm/paging_tmpl.h
>>> +++ b/arch/x86/kvm/paging_tmpl.h
>>> @@ -150,7 +150,9 @@ walk:
>>>   		walker->table_gfn[walker->level - 1] = table_gfn;
>>>   		walker->pte_gpa[walker->level - 1] = pte_gpa;
>>>
>>> -		kvm_read_guest(vcpu->kvm, pte_gpa,&pte, sizeof(pte));
>>> +		if (kvm_read_guest(vcpu->kvm, pte_gpa,&pte, sizeof(pte)))
>>> +			goto not_present;
>>> +
>>>        
>> On real hardware, if you place a pte at non-existing memory, you
>> aren't guaranteed to get the present bit clear, so why is this
>> necessary?
>>
>> We should be able to survive any garbage the pte previously contained.
>>      
> The problem is the content of the previous pte is processed (which is
> valid), but the cmpxchg fails (see the loop), without level decreasing.
>
>    

Yes, you're right.  Patch is fine, then (we could triple-fault instead 
of #PF, but I don't think it matters).

-- 
error compiling committee.c: too many arguments to function