linux-kernel - Re: [PATCH] x86/kvm/mmu: reset MMU context when 32-bit guest switches PAE

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, 30 Apr 2019 11:42:29 -0700
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>
Subject: Re: [PATCH] x86/kvm/mmu: reset MMU context when 32-bit guest
 switches PAE

On Tue, Apr 30, 2019 at 07:33:26PM +0200, Vitaly Kuznetsov wrote:
> Commit 47c42e6b4192 ("KVM: x86: fix handling of role.cr4_pae and rename it
> to 'gpte_size'") introduced a regression: 32-bit PAE guests stopped

"gpte_is_8_bytes" is really confusing in this case. :-(  Unfortunately I
can't think I can't think of a better name that isn't ridiculously verbose.

> working. The issue appears to be: when guest switches (enables) PAE we need
> to re-initialize MMU context (set context->root_level, do
> reset_rsvds_bits_mask(), ...) but init_kvm_tdp_mmu() doesn't do that
> because we threw away is_pae(vcpu) flag from mmu role. Restore it to
> kvm_mmu_extended_role (as we now don't need it in base role) to fix
> the issue.

The change makes sense, but I'm amazed that there's a kernel that can
actually trigger the bug.  The extended role tracks CR0.PG, so I'm pretty
sure hitting this bug requires toggling CR4.PAE *while paging is enabled*.
Which is legal, but crazy.  E.g. my 32-bit Linux VM runs fine with and
without PAE enabled.

> Fixes: 47c42e6b4192 ("KVM: x86: fix handling of role.cr4_pae and rename it to 'gpte_size'")
> Signed-off-by: Vitaly Kuznetsov <vkuznets@...hat.com>

Reviewed-by: Sean Christopherson <sean.j.christopherson@...el.com>

> ---
> - RFC: it was proven multiple times that mmu code is more complex than it
>   appears (at least to me) :-)

LOL, maybe you're just more optimistic than most people.  Every time I
look at the code I say something along the lines of "holy $&*#".

> ---
>  arch/x86/include/asm/kvm_host.h | 1 +
>  arch/x86/kvm/mmu.c              | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index a9d03af34030..c79abe7ca093 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -295,6 +295,7 @@ union kvm_mmu_extended_role {
>  		unsigned int valid:1;
>  		unsigned int execonly:1;
>  		unsigned int cr0_pg:1;
> +		unsigned int cr4_pae:1;
>  		unsigned int cr4_pse:1;
>  		unsigned int cr4_pke:1;
>  		unsigned int cr4_smap:1;
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index e10962dfc203..d9c7b45d231f 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -4781,6 +4781,7 @@ static union kvm_mmu_extended_role kvm_calc_mmu_role_ext(struct kvm_vcpu *vcpu)
>  	union kvm_mmu_extended_role ext = {0};
>  
>  	ext.cr0_pg = !!is_paging(vcpu);
> +	ext.cr4_pae = !!is_pae(vcpu);

This got me thinking, I wonder if we can/should leave the CR4 bits clear
if !is_paging().  Technically I think we're unnecessarily purging the MMU
when the guest is toggling CR4 bits with CR0.PG==0.  And I think we can
also git rid of the oddball nx flag in struct kvm_mmu.  I'll play around
with the code and hopefully send a patch or two.

>  	ext.cr4_smep = !!kvm_read_cr4_bits(vcpu, X86_CR4_SMEP);
>  	ext.cr4_smap = !!kvm_read_cr4_bits(vcpu, X86_CR4_SMAP);
>  	ext.cr4_pse = !!is_pse(vcpu);
> -- 
> 2.20.1
>