linux-kernel - Re: [PATCH v4 5/6] KVM: MMU: combine guest pte read between walk and pte prefetch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4C2F2835.5060508@redhat.com>
Date:	Sat, 03 Jul 2010 15:08:21 +0300
From:	Avi Kivity <avi@...hat.com>
To:	Xiao Guangrong <xiaoguangrong@...fujitsu.com>
CC:	Marcelo Tosatti <mtosatti@...hat.com>,
	LKML <linux-kernel@...r.kernel.org>,
	KVM list <kvm@...r.kernel.org>
Subject: Re: [PATCH v4 5/6] KVM: MMU: combine guest pte read between walk
 and pte prefetch

On 07/03/2010 01:31 PM, Xiao Guangrong wrote:
>
>> See how the pte is reread inside fetch with mmu_lock held.
>>
>>      
> It looks like something is broken in 'fetch' functions, this patch will
> fix it.
>
> Subject: [PATCH] KVM: MMU: fix last level broken in FNAME(fetch)
>
> We read the guest level out of 'mmu_lock', sometimes, the host mapping is
> confusion. Consider this case:
>
> VCPU0:                                              VCPU1
>
> Read guest mapping, assume the mapping is:
> GLV3 ->  GLV2 ->  GLV1 ->  GFNA,
> And in the host, the corresponding mapping is
> HLV3 ->  HLV2 ->  HLV1(P=0)
>
>                                                     Write GLV1 and cause the
>                                                     mapping point to GFNB
>                                                     (May occur in pte_write or
>                                                        invlpg path)
>
> Mapping GLV1 to GFNA
>
> This issue only occurs in the last indirect mapping, since if the middle
> mapping is changed, the mapping will be zapped, then it will be detected
> in the FNAME(fetch) path, but when it map the last level, it not checked.
>
> Fixed by also check the last level.
>
>    

I don't really see what is fixed.  We already check the gpte.  What's 
special about the new scenario?

> @@ -322,6 +334,12 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
>   		level = iterator.level;
>   		sptep = iterator.sptep;
>   		if (iterator.level == hlevel) {
> +			if (check&&  level == gw->level&&
> +			      !FNAME(check_level_mapping)(vcpu, gw, hlevel)) {
> +				kvm_release_pfn_clean(pfn);
> +				break;
> +			}
> +
>    

Now we check here...

>   			mmu_set_spte(vcpu, sptep, access,
>   				     gw->pte_access&  access,
>   				     user_fault, write_fault,
> @@ -376,10 +394,10 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
>   		sp = kvm_mmu_get_page(vcpu, table_gfn, addr, level-1,
>   					       direct, access, sptep);
>   		if (!direct) {
> -			r = kvm_read_guest_atomic(vcpu->kvm,
> -						  gw->pte_gpa[level - 2],
> -						&curr_pte, sizeof(curr_pte));
> -			if (r || curr_pte != gw->ptes[level - 2]) {
> +			if (hlevel == level - 1)
> +				check = false;
> +
> +			if (!FNAME(check_level_mapping)(vcpu, gw, level - 1)) {
>    

... and here?  Why?


(looking at the code, we have a call to kvm_host_page_size() on every 
page fault, that takes mmap_sem... that's got to impact scaling)

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/