From: Lyle Li <LyleLi@zhaoxin.com>

The callers of the xfeatures_in_use function must ensure that the
current processor has the X86_FEATURE_XGETBV1 feature. However, in some
places where xfeatures_in_use is called, there is no check to see if the
processor supports this feature, leading to the execution of the XGETBV
XCR1 instruction on processors that do not support this feature,
triggering a #GP exception, and ultimately causing an OS panic.

To fix this issue, a check for the X86_FEATURE_XGETBV1 feature has been
added before calling xfeatures_in_use.

Fixes: ae6012d72fa6 ("x86/pkeys: Ensure updated PKRU value is XRSTOR'd") 
Fixes: 30d02551ba4f ("x86/fpu: Optimize out sigframe xfeatures when in init state")
Signed-off-by: Lyle Li <LyleLi@zhaoxin.com>
Signed-off-by: Tony W Wang-oc <TonyWWang-oc@zhaoxin.com>
---
 arch/x86/kernel/fpu/xstate.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
index aa16f1a1b..4d966c6c7 100644
--- a/arch/x86/kernel/fpu/xstate.h
+++ b/arch/x86/kernel/fpu/xstate.h
@@ -80,6 +80,9 @@ static inline int update_pkru_in_sigframe(struct xregs_state __user *buf, u64 ma
 	if (unlikely(!cpu_feature_enabled(X86_FEATURE_OSPKE)))
 		return 0;
 
+	if (!cpu_feature_enabled(X86_FEATURE_XGETBV1))
+		return 0;
+
 	/* Mark PKRU as in-use so that it is restored correctly. */
 	xstate_bv = (mask & xfeatures_in_use()) | XFEATURE_MASK_PKRU;
 
@@ -292,7 +295,7 @@ static inline int xsave_to_user_sigframe(struct xregs_state __user *buf, u32 pkr
 	int err;
 
 	/* Optimize away writing unnecessary xfeatures: */
-	if (fpu_state_size_dynamic())
+	if (cpu_feature_enabled(X86_FEATURE_XGETBV1) && fpu_state_size_dynamic())
 		mask &= xfeatures_need_sigframe_write();
 
 	lmask = mask;
-- 
2.25.1

On 1/1/2025 11:54 PM, Tony W Wang-oc wrote:
> From: Lyle Li <LyleLi@zhaoxin.com>
> 
> The callers of the xfeatures_in_use function must ensure that the
> current processor has the X86_FEATURE_XGETBV1 feature. However, in some
> places where xfeatures_in_use is called, there is no check to see if the
> processor supports this feature, leading to the execution of the XGETBV
> XCR1 instruction on processors that do not support this feature,
> triggering a #GP exception, and ultimately causing an OS panic.

I doubt this is a real issue. An XFD implementation without XGETBV1 is 
considerably broken; every AMX system includes XGETBV1. Similarly, as 
far as I can see, PKU implementations also include XGETBV1. QEMU's CPU 
feature list [1] seems consistent with this.

Maybe a wild clearcpuid use may clear off the XGETBV1 flag. Adding this 
dependency to the table would make the relationship explicit:

static const struct cpuid_dep cpuid_deps[] = {
...
+       { X86_FEATURE_PKU,                      X86_FEATURE_XGETBV1   },
         {}
  };

Note that XFD is already listed as dependent on XGETBV1.

But I doubt the kernel needs to be resilient to deliberately 
misconfigured or crazy virtual machine setups.

Thanks,
Chang

[1] https://github.com/qemu/qemu/blob/master/target/i386/cpu.c

On Wed, Jan 15, 2025, Chang S. Bae wrote:
> On 1/1/2025 11:54 PM, Tony W Wang-oc wrote:
> > From: Lyle Li <LyleLi@zhaoxin.com>
> > 
> > The callers of the xfeatures_in_use function must ensure that the
> > current processor has the X86_FEATURE_XGETBV1 feature. However, in some
> > places where xfeatures_in_use is called, there is no check to see if the
> > processor supports this feature, leading to the execution of the XGETBV
> > XCR1 instruction on processors that do not support this feature,
> > triggering a #GP exception, and ultimately causing an OS panic.
> 
> I doubt this is a real issue. An XFD implementation without XGETBV1 is
> considerably broken; every AMX system includes XGETBV1. Similarly, as far as
> I can see, PKU implementations also include XGETBV1. QEMU's CPU feature list
> [1] seems consistent with this.

QEMU's CPU models are not authoritative when it comes to architecture, they are
purely what CPUs hardware vendors have shipped.  The absense of a model with PKU
but not XGETBV1 simply reflects that neither Intel nor AMD have ever shipped such
a model.

> Maybe a wild clearcpuid use may clear off the XGETBV1 flag. Adding this
> dependency to the table would make the relationship explicit:
> 
> static const struct cpuid_dep cpuid_deps[] = {
> ...
> +       { X86_FEATURE_PKU,                      X86_FEATURE_XGETBV1   },
>         {}
>  };
> 
> Note that XFD is already listed as dependent on XGETBV1.
> 
> But I doubt the kernel needs to be resilient to deliberately misconfigured
> or crazy virtual machine setups.

I don't see anything in the SDM that suggests this is a misconfigured CPU.  Intel
might not have plans to ship such CPUs, but AFAICT it's not a violation of the
architecture as defined in the SDM.

The SDM even explicitly says that protection keys can exist and be used without
PKU state being supported in XSAVE at all, at which point assuming the existence
of XGETBV1 is rather nonsensical.

  XCR0[9] is associated with PKRU state (see Section 13.5.7). Software can use
  the XSAVE feature set to manage PKRU state only if XCR0[9] = 1. The value of
  XCR0[9] in no way determines whether software can use protection keys or execute
  other instructions that access PKRU state (these instructions can be executed even
  if XCR0[9] = 0).

  XCR0[9] is 0 coming out of RESET. As noted in Section 13.2, a processor allows
  software to set XCR0[9] if and only if CPUID.(EAX=0DH,ECX=0):EAX[9] = 1.

On 1/17/25 14:10, Sean Christopherson wrote:
>>
>> Note that XFD is already listed as dependent on XGETBV1.
>>
>> But I doubt the kernel needs to be resilient to deliberately misconfigured
>> or crazy virtual machine setups.
> 
> I don't see anything in the SDM that suggests this is a misconfigured CPU.  Intel
> might not have plans to ship such CPUs, but AFAICT it's not a violation of the
> architecture as defined in the SDM.
> 
> The SDM even explicitly says that protection keys can exist and be used without
> PKU state being supported in XSAVE at all, at which point assuming the existence
> of XGETBV1 is rather nonsensical.
> 

Whether or not a combination is *possible* at all is totally separate 
from whether or not it is worth for Linux to support it. The CPUID 
dependency list exists for exactly that reason -- it defines *Linux 
policy* with regards to feature dependencies.

	-hpa

On 1/17/2025 2:10 PM, Sean Christopherson wrote:
> 
> I don't see anything in the SDM that suggests this is a misconfigured CPU.  Intel
> might not have plans to ship such CPUs, but AFAICT it's not a violation of the
> architecture as defined in the SDM.
> 
> The SDM even explicitly says that protection keys can exist and be used without
> PKU state being supported in XSAVE at all, at which point assuming the existence
> of XGETBV1 is rather nonsensical.
> 
>    XCR0[9] is associated with PKRU state (see Section 13.5.7). Software can use
>    the XSAVE feature set to manage PKRU state only if XCR0[9] = 1. The value of
>    XCR0[9] in no way determines whether software can use protection keys or execute
>    other instructions that access PKRU state (these instructions can be executed even
>    if XCR0[9] = 0).
> 
>    XCR0[9] is 0 coming out of RESET. As noted in Section 13.2, a processor allows
>    software to set XCR0[9] if and only if CPUID.(EAX=0DH,ECX=0):EAX[9] = 1.

Yeah, right.

Furthermore, looking further at the update_pkru_in_sigframe() change, I 
doubt the xfeatures_in_use() invocation is really necessary.

The change seems to always write the PKRU state in the signal frame and 
set the PKRU bit. Since XSAVE has already been performed, the latter 
could be something like this:

	__get_user(xstate_bv, &buf->header.xfeatures);
	xstate_bv |= XFEATURE_MASK_PKRU;
	__put_user(xstate_bv, &buf->header.xfeatures);

A similar code is already there for FP/SSE bits in the 
save_xstate_epilog() function:
	https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/fpu/signal.c#n139

Thanks,
Chang