Re: [PATCH] x86/fpu/xstate: clear XSAVE features if DISABLED_MASK set

[Jon Kohler]

> On May 31, 2023, at 4:18 PM, Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> 
> On Wed, May 31, 2023, Jon Kohler wrote:
>> 
>>> On May 31, 2023, at 12:30 PM, Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>>> Caveat #1: cpu_feature_enabled() has a flaw that's relevant to this code: in the
>>> unlikely scenario that the compiler doesn't resolve "cid" to a compile-time
>>> constant value, cpu_feature_enabled() won't query DISABLED_MASK_BIT_SET().  I don't
>>> see any other use of cpu_feature_enabled() without a hardcoded X86_FEATURE_*, and
>>> the below compiles with my config, so I think/hope we can just require a compile-time
>>> constant when using cpu_feature_enabled().
>>> 
>> 
>> Yea I think that should work. I’ll club that into v2 of this patch.
> 
> I recommend doing it as a separate patch, hardening cpu_feature_enabled() shouldn't
> have a dependency on tweaking the xfeatures mask.  I tested this with an allyesconfig
> if you want to throw it in as a prep patch.

Ack, I’ll do that and make this into a small series, thanks for the help!

> 
> ---
> From: Sean Christopherson <seanjc@xxxxxxxxxx>
> Date: Wed, 31 May 2023 09:41:12 -0700
> Subject: [PATCH] x86/cpufeature: Require compile-time constant in
> cpu_feature_enabled()
> 
> Assert that the to-be-checked bit passed to cpu_feature_enabled() is a
> compile-time constant instead of applying the DISABLED_MASK_BIT_SET()
> logic if and only if the bit is a constant.  Conditioning the check on
> the bit being constant instead of requiring the bit to be constant could
> result in compiler specific kernel behavior, e.g. running on hardware that
> supports a disabled feature would return %false if the compiler resolved
> the bit to a constant, but %true if not.
> 
> All current usage of cpu_feature_enabled() specifies a hardcoded
> X86_FEATURE_* flag, so this *should* be a glorified nop.
> 
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
> arch/x86/include/asm/cpufeature.h | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
> index ce0c8f7d3218..886200fbf8d9 100644
> --- a/arch/x86/include/asm/cpufeature.h
> +++ b/arch/x86/include/asm/cpufeature.h
> @@ -141,8 +141,11 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
>  * supporting a possible guest feature where host support for it
>  * is not relevant.
>  */
> -#define cpu_feature_enabled(bit)	\
> -	(__builtin_constant_p(bit) && DISABLED_MASK_BIT_SET(bit) ? 0 : static_cpu_has(bit))
> +#define cpu_feature_enabled(bit) 				\
> +({								\
> +	BUILD_BUG_ON(!__builtin_constant_p(bit));		\
> +	DISABLED_MASK_BIT_SET(bit) ? 0 : static_cpu_has(bit);	\
> +})
> 
> #define boot_cpu_has(bit)	cpu_has(&boot_cpu_data, bit)
> 
> 
> base-commit: 39428f6ea9eace95011681628717062ff7f5eb5f
> -- 
> 
>>> I'm totally ok gating xfeature bits on cpu_feature_enabled(), but there should be
>>> a prep patch for KVM to clear features bits in kvm_cpu_caps if the corresponding
>>> XCR0/XSS bit is not set in the host.  If KVM ever wants to expose an xstate feature
>>> (other than MPX) that's disabled in the host, then we can revisit
>>> fpu__init_system_xstate().  But we need to ensure the "failure" mode is that
>>> KVM doesn't advertise the feature, as opposed to advertising a feature without
>>> without context switching its data.
>> 
>> 
>> Looking into this, trying to understand the comment about a feature being used
>> without context switching its data. 
>> 
>> In __kvm_x86_vendor_init() we’re already populating host_xcr0 using the 
>> XCR_XFEATURE_ENABLED_MASK, which should be populated on boot
>> by fpu__init_cpu_xstate(), which happens almost immediately after the code that I
>> modified in this commit. 
>> 
>> That then flows into guest_supported_xcr0 (as well as user_xfeatures). 
>> guest_supported_xcr0 is then plumbed into __kvm_set_xcr, which specifically says
>> that we’re using that to prevent the guest from setting bits that we won’t save in the
>> first place.
>> 
>>    /*
>>     * Do not allow the guest to set bits that we do not support
>>     * saving.  However, xcr0 bit 0 is always set, even if the
>>     * emulated CPU does not support XSAVE (see kvm_vcpu_reset()).
>>     */
>>    valid_bits = vcpu->arch.guest_supported_xcr0 | XFEATURE_MASK_FP;
>> 
>> Wouldn’t this mean that the *guest* xstate initialization would not see a given
>> feature too and take care of the problem naturally?
>> 
>> Or are you saying you’d want an even more detailed clearing?
> 
> 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);
    }
...
}