linux-kernel - Re: [RFC PATCH v5 037/104] KVM: x86/mmu: Allow non-zero init value for shadow PTE

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:   Tue, 5 Apr 2022 16:13:33 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Kai Huang <kai.huang@...el.com>, isaku.yamahata@...el.com,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     isaku.yamahata@...il.com, Jim Mattson <jmattson@...gle.com>,
        erdemaktas@...gle.com, Connor Kuehl <ckuehl@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>
Subject: Re: [RFC PATCH v5 037/104] KVM: x86/mmu: Allow non-zero init value
 for shadow PTE

On 4/1/22 07:13, Kai Huang wrote:
>> @@ -617,9 +617,9 @@ static int mmu_spte_clear_track_bits(struct kvm *kvm, u64 *sptep)
>>   	int level = sptep_to_sp(sptep)->role.level;
>>   
>>   	if (!spte_has_volatile_bits(old_spte))
>> -		__update_clear_spte_fast(sptep, 0ull);
>> +		__update_clear_spte_fast(sptep, shadow_init_value);
>>   	else
>> -		old_spte = __update_clear_spte_slow(sptep, 0ull);
>> +		old_spte = __update_clear_spte_slow(sptep, shadow_init_value);

(FWIW this one should also assume that the init_value is zero, and WARN 
if not).

> I guess it's better to have some comment here.  Allow non-zero init value for
> shadow PTE doesn't necessarily mean the initial value should be used when one
> PTE is zapped.  I think mmu_spte_clear_track_bits() is only called for mapping
> of normal (shared) memory but not MMIO? Then perhaps it's better to have a
> comment to explain we want "suppress #VE" set to get a real EPT violation for
> normal memory access from guest?
> 
>>   
>>   	if (!is_shadow_present_pte(old_spte))
>>   		return old_spte;
>> @@ -651,7 +651,7 @@ static int mmu_spte_clear_track_bits(struct kvm *kvm, u64 *sptep)
>>    */
>>   static void mmu_spte_clear_no_track(u64 *sptep)
>>   {
>> -	__update_clear_spte_fast(sptep, 0ull);
>> +	__update_clear_spte_fast(sptep, shadow_init_value);
>>   }
> Similar here.  Seems mmu_spte_clear_no_track() is used to zap non-leaf PTE which
> doesn't require state tracking, so theoretically it can be set to 0.  But this
> seems is also called to zap MMIO PTE so looks need to set to shadow_init_value.
> Anyway looks deserve a comment?
> 
> Btw, Above two changes to mmu_spte_clear_track_bits() and
> mmu_spte_clear_track_bits() seems a little bit out-of-scope of what this patch
> claims to do.  Allow non-zero init value for shadow PTE doesn't necessarily mean
> the initial value should be used when one PTE is zapped. Maybe we can further
> improve the patch title and commit message a little bit.  Such as: Allow non-
> zero value for empty (or invalid?) PTE? Non-present seems doesn't fit here.

I think shadow_init_value is not named well.  Let's rename it to 
shadow_nonpresent_value, and the commit to "Allow non-zero value for 
non-present SPTE".  That explains why it's used for zapping.

Paolo