linux-kernel - Re: [PATCH v2 12/25] KVM: x86/mmu: cleanup computation of MMU roles for two-dimensional paging

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, 8 Mar 2022 19:24:57 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        dmatlack@...gle.com
Subject: Re: [PATCH v2 12/25] KVM: x86/mmu: cleanup computation of MMU roles
 for two-dimensional paging

On 3/8/22 19:11, Sean Christopherson wrote:
> On Mon, Feb 21, 2022, Paolo Bonzini wrote:
>> Extended bits are unnecessary because page walking uses the CPU mode,
>> and EFER.NX/CR0.WP can be set to one unconditionally---matching the
>> format of shadow pages rather than the format of guest pages.
> 
> But they don't match the format of shadow pages.  EPT has an equivalent to NX in
> that KVM can always clear X, but KVM explicitly supports running with EPT and
> EFER.NX=0 in the host (32-bit non-PAE kernels).

In which case bit 2 of EPTs doesn't change meaning, does it?

> CR0.WP equally confusing.  Yes, both EPT and NPT enforce write protection at all
> times, but EPT has no concept of user vs. supervisor in the EPT tables themselves,
> at least with respect to writes (thanks mode-based execution for the qualifier...).
> NPT is even worse as the APM explicitly states:
> 
>    The host hCR0.WP bit is ignored under nested paging.
> 
> Unless there's some hidden dependency I'm missing, I'd prefer we arbitrarily leave
> them zero.

Setting EFER.NX=0 might be okay for EPT/NPT, but I'd prefer to set it 
respectively to 1 (X bit always present) and host EFER.NX (NX bit 
present depending on host EFER).

For CR0.WP it should really be 1 in my opinion, because CR0.WP=0 implies 
having a concept of user vs. supervisor access: CR0.WP=1 is the 
"default", while CR0.WP=0 is "always allow *supervisor* writes".

>> even if only barely so, due to SMM and guest mode; for consistency,
>> pass it down to kvm_calc_tdp_mmu_root_page_role instead of querying
>> the vcpu with is_smm or is_guest_mode.
> 
> The changelog should call out this is a _significant_ change in behavior for KVM,
> as it allows reusing shadow pages with different guest MMU "role bits".

Good point!  It's safe and arguably clea{n,r}er, but it's still a pretty 
large change.

> E.g. if this lands after the changes to not unload MMUs on cr0/cr4
> emulation, it will be quite the functional change.
I expect this to land first, so that the part where we don't really 
agree on the implementation comes last and benefits from a more 
understandable core.

Paolo