Re: [PATCH 1/8] x86/fpu: Check for missing AVX and AVX-512 xstate bits

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The patch fails to clear AVX and AVX-512 CPU capabilities when the CPU natively enumerates AVX but omits XSAVE or FPU support, bypassing the new sanitization logic and leading to #UD panics.

Pre-existing issues:
- [Critical] Memory Protection Keys (PKU) isolation can be bypassed because the kernel enables X86_FEATURE_PKU without verifying the presence of XFEATURE_PKRU in xstate.
--

commit 0cd2494a6ed30cd9eb21bc480a6d7d789ab47e00
Author: Eric Biggers <[email protected]>

x86/fpu: Check for missing AVX and AVX-512 xstate bits

If the CPU declares AVX or AVX-512 support, verify that the
corresponding xstate bits are also set.  If not, warn and clear them.
This eliminates the perceived need for AVX and AVX-512 optimized code in
the kernel to call cpu_has_xfeatures().

> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index a7b6524a9dea2..7f7e62e4ebc5b 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -799,6 +799,23 @@ static u64 __init guest_default_mask(void)
>  	return ~(u64)XFEATURE_MASK_USER_DYNAMIC;
>  }
>  
> +/* Clear any X86_FEATURE_* used by the kernel whose xfeatures are missing. */
> +static void __init clear_cpu_caps_with_missing_xfeatures(u64 xfeatures)
> +{
> +	u64 mask;
> +
> +	mask = XFEATURE_MASK_FPSSE | XFEATURE_MASK_YMM;
> +	if (boot_cpu_has(X86_FEATURE_AVX) && (xfeatures & mask) != mask) {
> +		pr_err("x86/fpu: Disabling AVX support due to missing xstate features\n");
> +		setup_clear_cpu_cap(X86_FEATURE_AVX);
> +	}
> +	mask = XFEATURE_MASK_FPSSE | XFEATURE_MASK_YMM | XFEATURE_MASK_AVX512;
> +	if (boot_cpu_has(X86_FEATURE_AVX512F) && (xfeatures & mask) != mask) {
> +		pr_err("x86/fpu: Disabling AVX-512 support due to missing xstate features\n");
> +		setup_clear_cpu_cap(X86_FEATURE_AVX512F);
> +	}
> +}
> +

[Severity: Critical]
This is a pre-existing issue, but Memory Protection Keys (PKU) isolation
can be bypassed because we don't verify X86_FEATURE_PKU against the
presence of XFEATURE_PKRU in xstate here.

If a hypervisor reports X86_FEATURE_PKU in CPUID but masks out the
XFEATURE_PKRU xstate component, the kernel's setup_pku() enables CR4.PKE
and forces X86_FEATURE_OSPKE.  Because X86_FEATURE_OSPKE is artificially
forced, the feature is not cleared.

CR4.PKE=1 permits userspace to use WRPKRU, but because XCR0.PKRU=0,
xsaves/xrstors silently ignore the PKRU register during task context
switches, leading to state leakage across tasks.

Should we also validate PKU against missing xstate components in this
function to prevent isolation bypass?

>  /*
>   * Enable and initialize the xsave feature.
>   * Called once per system bootup.

[ ... ]

> @@ -855,6 +872,8 @@ void __init fpu__init_system_xstate(unsigned int legacy_size)
>  		goto out_disable;
>  	}
>  
> +	clear_cpu_caps_with_missing_xfeatures(fpu_kernel_cfg.max_features);
> +

[Severity: High]
Does this logic get bypassed if XSAVE or FPU support is missing?

Earlier in fpu__init_system_xstate(), there are early returns if
X86_FEATURE_FPU or X86_FEATURE_XSAVE are missing:

	if (!boot_cpu_has(X86_FEATURE_XSAVE)) {
		pr_info("x86/fpu: x87 FPU will use %s\n",
			boot_cpu_has(X86_FEATURE_FXSR) ? "FXSAVE" : "FSAVE");
		return;
	}

If a hypervisor natively enumerates AVX=1 but XSAVE=0 (such as QEMU
with -cpu host,-xsave), check_cpufeature_deps() only warns about unmet
dependencies without clearing them.  The lack of X86_FEATURE_XSAVE triggers
the early return, completely bypassing this new capability cleanup logic.

Since the kernel will then attempt to execute VEX-encoded AVX instructions
while CR4.OSXSAVE=0, will this result in an invalid opcode (#UD) panic?

>  	fpu_kernel_cfg.independent_features = fpu_kernel_cfg.max_features &
>  					      XFEATURE_MASK_INDEPENDENT;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1