From: Yang Weijiang <weijiang.yang@intel.com>

guest-only supervisor state bits should be __ONLY__ enabled for guest
fpstate, i.e., never for normal kernel fpstate. WARN_ONCE() if normal
kernel fpstate sees any of these features.

Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
 arch/x86/kernel/fpu/xstate.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
index 1418423bc4c9..f644647c0549 100644
--- a/arch/x86/kernel/fpu/xstate.h
+++ b/arch/x86/kernel/fpu/xstate.h
@@ -208,6 +208,8 @@ static inline void os_xsave(struct fpstate *fpstate)
 	WARN_ON_FPU(!alternatives_patched);
 	xfd_validate_state(fpstate, mask, false);
 
+	WARN_ON_FPU(!fpstate->is_guest && (mask & XFEATURE_MASK_SUPERVISOR_GUEST));
+
 	XSTATE_XSAVE(&fpstate->regs.xsave, lmask, hmask, err);
 
 	/* We should never fault when copying to a kernel buffer: */
-- 
2.46.1

On 3/18/2025 8:31 AM, Chao Gao wrote:
>   
> +	WARN_ON_FPU(!fpstate->is_guest && (mask & XFEATURE_MASK_SUPERVISOR_GUEST));

Did you check xfeatures_mask_supervisor()? I think you might want to 
introduce a similar wrapper to reference the enabled features 
(max_features) here.

Also, have you audited other code paths to ensure that no additional 
guard like this is needed? Can you summarize your audit process?

Thanks,
Chang

On Tue, Apr 01, 2025 at 10:17:08AM -0700, Chang S. Bae wrote:
>On 3/18/2025 8:31 AM, Chao Gao wrote:
>> +	WARN_ON_FPU(!fpstate->is_guest && (mask & XFEATURE_MASK_SUPERVISOR_GUEST));
>
>Did you check xfeatures_mask_supervisor()? I think you might want to
>introduce a similar wrapper to reference the enabled features (max_features)
>here.

Are you suggesting something like this:

static inline u64 xfeatures_mask_guest_supervisor(void)
{
	return fpu_kernel_cfg.max_features & XFEATURE_MASK_SUPERVISOR_GUEST;
}

and do
	WARN_ON_FPU(!fpstate->is_guest && (mask & xfeatures_mask_guest_supervisor()));

?

If so, I don't think it's necessary. The check in the current patch is actually
stricter and could catch more errors than the suggested one.

>
>Also, have you audited other code paths to ensure that no additional guard
>like this is needed? Can you summarize your audit process?

I didn't audit all code paths. I took over this patch and made only very slight
modifications to it. Anyway, thanks for asking this.

The goal is to ensure that guest-only _supervisor_ features are not enabled in
non-guest FPUs.

There are two potential solutions:

a) Check the enabled features during save/restore operations, i.e., when
   executing XSAVES/XRSTORS instructions. This patch adopts this solution, but
   it is partial.

XSAVES/XRSTORS instructions are executed in following places:

  1) os_xrstor_booting()
  2) xsaves()
  3) xrstors()
  4) os_xrstor_safe()
  5) os_xsave()
  6) os_xrstor()
  7) os_xrstor_supervisor()

#2 and #3 are not applicable because they handle independent features only,
which are not associated with guest or non-guest FPUs.

Checks can be applied to #1 and #4-#7, although #1 needs to be refactored first
to accept an fpstate argument.

b) Check the enabled features during initialization and reallocation, i.e.,
whenever fpstate->xfeatures is assigned some value in following functions:

  __fpstate_reset()
  __fpstate_guest_reset()
  fpu__init_system_xstate()
  fpstate_realloc()

We can add a check within these functions or integrate the check into
xstate_init_xcomp_bv(), as it is always called after fpstate->xfeatures is
updated.

IMO, option b) is slightly better because it can catch errors earlier than
option a), allowing the call trace to accurately reflect how the issue arose.

Chang, which option do you prefer, or do you have any other suggestions?

>
>Thanks,
>Chang

On 4/2/2025 7:30 AM, Chao Gao wrote:
> 
> The goal is to ensure that guest-only _supervisor_ features are not enabled in
> non-guest FPUs.

I think the common XSAVE path matters here. The other XSAVE paths — 
signal delivery and saving the LBR state — already handle supervisor 
states properly. Signal delivery excludes all of supervisor states from 
the RFBM, while LBR state saving triggers a warning if any non-LBR state 
is set in the RFBM. Given this, the guard seems good enough unless 
missing something.

Thanks,
Chang

On 4/3/25 17:02, Chang S. Bae wrote:
> On 4/2/2025 7:30 AM, Chao Gao wrote:
>> The goal is to ensure that guest-only _supervisor_ features are
>> not enabled in non-guest FPUs.
> 
> I think the common XSAVE path matters here. The other XSAVE paths — 
> signal delivery and saving the LBR state — already handle supervisor 
> states properly. Signal delivery excludes all of supervisor states
> from the RFBM, while LBR state saving triggers a warning if any non-
> LBR state is set in the RFBM. Given this, the guard seems good
> enough unless missing something.
This patch is looking more and more optional. There's no broad consensus
on the need for a warning or exactly what it should warn on.

I think it should be dropped.