linux-kernel - Re: [PATCH] x86/fpu/xstate: clear XSAVE features if DISABLED

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:   Mon, 5 Jun 2023 13:23:28 +0000
From:   Jon Kohler <jon@...anix.com>
To:     Sean Christopherson <seanjc@...gle.com>
CC:     Dave Hansen <dave.hansen@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        "Chang S. Bae" <chang.seok.bae@...el.com>,
        Kyle Huey <me@...ehuey.com>,
        "neelnatu@...gle.com" <neelnatu@...gle.com>,
        Andrew Cooper <andrew.cooper3@...rix.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH] x86/fpu/xstate: clear XSAVE features if DISABLED_MASK set



> On May 31, 2023, at 5:09 PM, Jon Kohler <jon@...anix.com> wrote:
> 
> 
>> 
>> The CPUID bits that enumerate support for a feature are independent from the CPUID
>> bits that enumerate what XCR0 bits are supported, i.e. what features can be saved
>> and restored via XSAVE/XRSTOR.
>> 
>> KVM does mostly account for host XCR0, but in a very ad hoc way.  E.g. MPX is
>> handled by manually checking host XCR0.
>> 
>> 	if (kvm_mpx_supported())
>> 		kvm_cpu_cap_check_and_set(X86_FEATURE_MPX);
>> 
>> PKU manually checks too, but indirectly by looking at whether or not the kernel
>> has enabled CR4.OSPKE.
>> 
>> 	if (!tdp_enabled || !boot_cpu_has(X86_FEATURE_OSPKE))
>> 		kvm_cpu_cap_clear(X86_FEATURE_PKU);
>> 
>> But unless I'm missing something, the various AVX and AMX bits rely solely on
>> boot_cpu_data, i.e. would break if someone added CONFIG_X86_AVX or CONFIG_X86_AMX.
> 
> What if we simply moved static unsigned short xsave_cpuid_features[] … into xstate.h, which
> is already included in arch/x86/kvm/cpuid.c, and do something similar to what I’m proposing in this
> patch already
> 
> This would future proof such breakages I’d imagine?
> 
> void kvm_set_cpu_caps(void)
> {
> ...
>    /*
>     * Clear CPUID for XSAVE features that are disabled.
>     */
>    for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) {
>        unsigned short cid = xsave_cpuid_features[i];
> 
>        /* Careful: X86_FEATURE_FPU is 0! */
>        if ((i != XFEATURE_FP && !cid) || !boot_cpu_has(cid) ||
>            !cpu_feature_enabled(cid))
>            kvm_cpu_cap_clear(cid);
>    }
> …
> }
> 

Sean - following up on this rough idea code above, wanted to validate that this was the 
direction you were thinking of having kvm_set_cpu_caps() clear caps when a particular
xsave feature was disabled?