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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Mon, 05 Jul 2010 16:45:47 +0800
From:	Xiao Guangrong <xiaoguangrong@...fujitsu.com>
To:	Avi Kivity <avi@...hat.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



Avi Kivity wrote:

>> Looks into the code more carefully, maybe this code is wrong:
>>
>>
>>               if (!direct) {
>>                       r = kvm_read_guest_atomic(vcpu->kvm,
>> -                                          gw->pte_gpa[level - 2],
>> +                                          gw->pte_gpa[level - 1],
>>                                        &curr_pte, sizeof(curr_pte));
>> -                    if (r || curr_pte != gw->ptes[level - 2]) {
>> +                    if (r || curr_pte != gw->ptes[level - 1]) {
>>                                  kvm_mmu_put_page(sp, sptep);
>>                                  kvm_release_pfn_clean(pfn);
>>                                  sptep = NULL;
>>
>> It should check the 'level' mapping not 'level - 1', in the later
>> description
>> i'll explain it.
>>    
> 
> Right, this fixes the check for the top level, but it removes a check
> from the bottom level.
> 

We no need check the bottom level if guest not modify the bottom level,
if guest modify it, the bottom level is no-present, it also can broke
Point A's judgment and be checked by 'Point C'

> We need to move this to the top of the loop so we check all levels.  I
> guess this is why you needed to add a new check point.  But since we
> loop at least glevels times, we don't need two check points.
> 
  
> 
> Ok.   So moving the check to before point A, and s/level - 2/level - 1/
> should work, yes?
> 
> Should be slightly simpler since we don't need to kvm_mmu_put_page(sp,
> sptep) any more.

Yeah, it can work, but check all levels is really unnecessary, if guest not
modify the level, the check can be avoid.

This is why i choose two check-point, one is behind Point A's judgment, this
point checks the level which modified by guest, and another point is at mapping
last level point, this check is alway need.

> 
> Thanks for the detailed explanation.
> 

It's really my pleasure :-)

--
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/