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

From: Sean Christopherson
Date: Wed May 31 2023 - 16:18:47 EST

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.

From: Sean Christopherson <seanjc@xxxxxxxxxx>
Date: Wed, 31 May 2023 09:41:12 -0700
Subject: [PATCH] x86/cpufeature: Require compile-time constant in

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())

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))

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.