Re: [PATCH v2 1/10] KVM: MMU: fix writable sync sp mapping

[Avi Kivity <avi@redhat.com>]

On 06/27/2010 10:59 AM, Xiao Guangrong wrote:
>
> Xiao Guangrong wrote:
>
>    
>>
>> -		/*
>> -		 * Optimization: for pte sync, if spte was writable the hash
>> -		 * lookup is unnecessary (and expensive). Write protection
>> -		 * is responsibility of mmu_get_page / kvm_sync_page.
>> -		 * Same reasoning can be applied to dirty page accounting.
>> -		 */
>> -		if (!can_unsync&&  is_writable_pte(*sptep))
>> -			goto set_pte;
>> -
>>      
> Sorry, this optimization not broken anything, just my mistake, please review
> this.
>
> Subject: [PATCH v2 1/10] KVM: MMU: fix writable sync sp mapping
>
> While we sync the unsync sp, we may mapping the spte writable, it's
> dangerous, if one unsync sp's mapping gfn is another unsync page's gfn.
>
> For example:
> have two unsync pages SP1, SP2 and:
>
> SP1.pte[0] = P
> SP2.gfn's pfn = P
> [SP1.pte[0] = SP2.gfn's pfn]
>
> First, we unsync SP2, it will write protect for SP2.gfn since
>    

Do you mean we sync SP2 here?

> SP1.pte[0] is mapping to this page, it will mark read only.
>
> Then, we unsync SP1, SP1.pte[0] may mark to writable.
>    

How can unsyncing SP1 change SP1.pte[0]?

When we unsync SP2 by a fault through SP1.pte[0], that can cause 
SP1.pte[0] to become writable.  But unsyncing SP1 shouldn't have an 
effect on its sptes.

> Now, we will write SP2.gfn by SP1.pte[0] mapping
>
> This bug will corrupt guest's page table, fixed by mark read-only mapping
> if the mapped gfn has shadow page
>
> Signed-off-by: Xiao Guangrong<xiaoguangrong@cn.fujitsu.com>
> ---
>   arch/x86/kvm/mmu.c |    5 ++++-
>   1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 045a0f9..24290f8 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -1810,11 +1810,14 @@ static int mmu_need_write_protect(struct kvm_vcpu *vcpu, gfn_t gfn,
>   	bool need_unsync = false;
>
>   	for_each_gfn_indirect_valid_sp(vcpu->kvm, s, gfn, node) {
> +		if (!can_unsync)
> +			return 1;
> +
>    

What if the page is already unsync?  We don't need write protection in 
this case.

>   		if (s->role.level != PT_PAGE_TABLE_LEVEL)
>   			return 1;
>
>   		if (!need_unsync&&  !s->unsync) {
> -			if (!can_unsync || !oos_shadow)
> +			if (!oos_shadow)
>   				return 1;
>   			need_unsync = true;
>   		}
>    

How can this change anything?  On the first pass, need_unsync  = false, 
so we will check can_unsync and return.

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