linux-kernel - Re: [PATCH 1/3] x86/cpu: Process all CPUID dependencies after identifying CPU info

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: <Y5INU3o+SFReGkLz@zn.tnic>
Date:   Thu, 8 Dec 2022 17:14:11 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Subject: Re: [PATCH 1/3] x86/cpu: Process all CPUID dependencies after
 identifying CPU info

On Sat, Dec 03, 2022 at 12:37:43AM +0000, Sean Christopherson wrote:
> Process all CPUID dependencies to ensure that a dependent is disabled if
> one or more of its parent features is unsupported.

Just out of curiosity: this is some weird guest configuration, right?
Not addressing a real hw issue...

> As is, cpuid_deps is
> processed if an only if a feature is explicitly disabled via
> clear_cpu_cap(), which makes it annoying/dangerous to use cpuid_deps for
> features whose parent(s) do not always have explicit processing.
> 
> E.g. VMX and SGX depend on the synthetic X86_FEATURE_MSR_IA32_FEAT_CTL,
> but there is no common location to clear MSR_IA32_FEAT_CTL, and so
> consumers of VMX and SGX are forced to check MSR_IA32_FEAT_CTL on top
> of the dependent feature.
> 
> Manually clearing X86_FEATURE_MSR_IA32_FEAT_CTL is the obvious
> alternative, but it's subtly more difficult that updating
> init_ia32_feat_ctl().  CONFIG_IA32_FEAT_CTL depends on any of
> CONFIG_CPU_SUP_{INTEL,CENATUR,ZHAOXIN}, but init_ia32_feat_ctl() is
> invoked if and only if the actual CPU type matches one of the
> aforementioned CPU_SUP_* types. E.g. running a kernel built with
> 
>   CONFIG_CPU_SUP_INTEL=y
>   CONFIG_CPU_SUP_AMD=y
>   # CONFIG_CPU_SUP_HYGON is not set
>   # CONFIG_CPU_SUP_CENTAUR is not set
>   # CONFIG_CPU_SUP_ZHAOXIN is not set
> 
> on a Cenatur or Zhaoxin CPU will leave X86_FEATURE_VMX set but not set

Typo fix for the committer: Centaur

> X86_FEATURE_MSR_IA32_FEAT_CTL, and will never call init_ia32_feat_ctl()
> to give the kernel a convenient opportunity to clear
> X86_FEATURE_MSR_IA32_FEAT_CTL.
> 
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/include/asm/cpufeature.h |  1 +
>  arch/x86/kernel/cpu/common.c      |  6 ++++++
>  arch/x86/kernel/cpu/cpuid-deps.c  | 10 ++++++++++
>  3 files changed, 17 insertions(+)

...

> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index bf4ac1cb93d7..094fc69dba63 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1887,6 +1887,12 @@ static void identify_cpu(struct cpuinfo_x86 *c)
>  
>  	ppin_init(c);
>  
> +	/*
> +	 * Apply CPUID dependencies to ensure dependent features are disabled
> +	 * if a parent feature is unsupported but wasn't explicitly disabled.
> +	 */
> +	apply_cpuid_deps(c);

I'd probably call that resolve_cpuid_deps()...

But yeah, that init path would need cleaning up at some point - all
kinds of init detection functions have been hastily slapped there over
the years...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette