linux-kernel - Re: [PATCH V3 11/12] KVM: X86/MMU: Don't use mmu->pae_root when shadowing PAE NPT in 64-bit host

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]

Message-ID: <Ytc9j/ayzTfm6Rti@google.com>
Date:   Tue, 19 Jul 2022 23:26:07 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Lai Jiangshan <jiangshanlai@...il.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Maxim Levitsky <mlevitsk@...hat.com>,
        David Matlack <dmatlack@...gle.com>,
        Lai Jiangshan <jiangshan.ljs@...group.com>
Subject: Re: [PATCH V3 11/12] KVM: X86/MMU: Don't use mmu->pae_root when
 shadowing PAE NPT in 64-bit host

On Sat, May 21, 2022, Lai Jiangshan wrote:
> From: Lai Jiangshan <jiangshan.ljs@...group.com>
> 
> Allocate the tables when allocating the local shadow page.

This absolutely needs a much more verbose changelog.

> Signed-off-by: Lai Jiangshan <jiangshan.ljs@...group.com>
> ---
>  arch/x86/kvm/mmu/mmu.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 63c2b2c6122c..73e6a8e1e1a9 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -1809,10 +1809,12 @@ static bool using_local_root_page(struct kvm_mmu *mmu)
>   * 2 or 3 levels of local shadow pages on top of non-local shadow pages.
>   *
>   * Local shadow pages are locally allocated.  If the local shadow page's level
> - * is PT32E_ROOT_LEVEL, it will use the preallocated mmu->pae_root for its
> - * sp->spt.  Because sp->spt may need to be put in the 32 bits CR3 (even in
> - * x86_64) or decrypted.  Using the preallocated one to handle these
> - * requirements makes the allocation simpler.
> + * is PT32E_ROOT_LEVEL, and it is not shadowing nested NPT for 32-bit L1 in
> + * 64-bit L0 (or said when the shadow pagetable's level is PT32E_ROOT_LEVEL),
> + * it will use the preallocated mmu->pae_root for its sp->spt.  Because sp->spt
> + * need to be put in the 32-bit CR3 (even in 64-bit host) or decrypted.  Using
> + * the preallocated one to handle these requirements makes the allocation
> + * simpler.
>   *
>   * Local shadow pages are only visible to local VCPU except through
>   * sp->parent_ptes rmap from their children, so they are not in the
> @@ -1852,13 +1854,12 @@ kvm_mmu_alloc_local_shadow_page(struct kvm_vcpu *vcpu, union kvm_mmu_page_role r
>  	sp->gfn = 0;
>  	sp->role = role;
>  	/*
> -	 * Use the preallocated mmu->pae_root when the shadow page's
> -	 * level is PT32E_ROOT_LEVEL which may need to be put in the 32 bits
> +	 * Use the preallocated mmu->pae_root when the shadow pagetable's
> +	 * level is PT32E_ROOT_LEVEL which need to be put in the 32 bits
>  	 * CR3 (even in x86_64) or decrypted.  The preallocated one is prepared
>  	 * for the requirements.
>  	 */
> -	if (role.level == PT32E_ROOT_LEVEL &&
> -	    !WARN_ON_ONCE(!vcpu->arch.mmu->pae_root))

Why remove this WARN_ON_ONCE()?  And shouldn't this also interact with 

   KVM: X86/MMU: Allocate mmu->pae_root for PAE paging on-demand

Actually, I think the series is buggy.  That patch, which precedes this one, does

	if (vcpu->arch.mmu->root_role.level != PT32E_ROOT_LEVEL)
		return 0;

i.e. does NOT allocate pae_root for a 64-bit host, which means that running KVM
against the on-demand patch would result in the WARN firing and bad things happening.

> +	if (vcpu->arch.mmu->root_role.level == PT32E_ROOT_LEVEL)
>  		sp->spt = vcpu->arch.mmu->pae_root;
>  	else
>  		sp->spt = kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_shadow_page_cache);
> -- 
> 2.19.1.6.gb485710b
>