linux-kernel - Re: [PATCH v2 36/66] KVM: x86: Handle GBPAGE CPUID adjustment for EPT in VMX code

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, 3 Mar 2020 07:35:50 -0800
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Xiaoyao Li <xiaoyao.li@...el.com>
Subject: Re: [PATCH v2 36/66] KVM: x86: Handle GBPAGE CPUID adjustment for
 EPT in VMX code

On Tue, Mar 03, 2020 at 03:59:14PM +0100, Paolo Bonzini wrote:
> On 03/03/20 00:56, Sean Christopherson wrote:
> > Move the clearing of the GBPAGE CPUID bit into VMX to eliminate an
> > instance of the undesirable "unsigned f_* = *_supported ? F(*) : 0"
> > pattern in the common CPUID handling code, and to pave the way toward
> > eliminating ->get_lpage_level().
> > 
> > No functional change intended.
> 
> And no functional change is done indeed but there is a preexisting bug 
> that should be fixed.
> 
> cpu_has_vmx_ept_1g_page() has no relationship to whether 1GB pages should be
> marked as supported in CPUID.  This has no ill effect because we're only
> clearing the bit, but it results in 1GB pages not being available when
> EPT is disabled (even though they are actually supported thanks to
> shadowing).

Oof, that took me a long time to process.  You're saying that KVM can
allow the guest to use GBPAGES when shadow paging is enabled because KVM
can effectively emulate GBPAGES.  And IIUC, you're also saying that
cpuid.GBPAGES should never be influenced by EPT restrictions.

That all makes sense.

> The right fix should be this:
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 84b9a488a443..8bbba8eb4ce5 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -416,8 +416,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>  	int r, i, max_idx;
>  	unsigned f_nx = is_efer_nx() ? F(NX) : 0;
>  #ifdef CONFIG_X86_64
> -	unsigned f_gbpages = (kvm_x86_ops->get_lpage_level() == PT_PDPE_LEVEL)
> -				? F(GBPAGES) : 0;
> +	unsigned f_gbpages = F(GBPAGES);
>  	unsigned f_lm = F(LM);
>  #else
>  	unsigned f_gbpages = 0;
> @@ -691,6 +690,8 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>  	case 0x80000001:
>  		entry->edx &= kvm_cpuid_8000_0001_edx_x86_features;
>  		cpuid_entry_mask(entry, CPUID_8000_0001_EDX);
> +		if (!tdp_enabled)
> +			cpuid_entry_set(entry, X86_FEATURE_GBPAGES);
>  		entry->ecx &= kvm_cpuid_8000_0001_ecx_x86_features;
>  		cpuid_entry_mask(entry, CPUID_8000_0001_ECX);
>  		break;
> 
> Paolo
>