Re: [PATCH v4 8/8] x86/fpu/xstate: Warn if guest-only supervisor states are detected in normal fpstate

[Chao Gao <chao.gao@intel.com>]

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