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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z+1KBN+s3CWdTN60@intel.com>
Date: Wed, 2 Apr 2025 22:30:28 +0800
From: Chao Gao <chao.gao@...el.com>
To: "Chang S. Bae" <chang.seok.bae@...el.com>
CC: <x86@...nel.org>, <linux-kernel@...r.kernel.org>, <kvm@...r.kernel.org>,
	<tglx@...utronix.de>, <dave.hansen@...el.com>, <seanjc@...gle.com>,
	<pbonzini@...hat.com>, <peterz@...radead.org>, <rick.p.edgecombe@...el.com>,
	<weijiang.yang@...el.com>, <john.allen@....com>, <bp@...en8.de>,
	<xin3.li@...el.com>, Ingo Molnar <mingo@...hat.com>, Dave Hansen
	<dave.hansen@...ux.intel.com>, "H. Peter Anvin" <hpa@...or.com>, "Aruna
 Ramakrishna" <aruna.ramakrishna@...cle.com>, Mitchell Levy
	<levymitchell0@...il.com>, Adamos Ttofari <attofari@...zon.de>, Uros Bizjak
	<ubizjak@...il.com>
Subject: Re: [PATCH v4 8/8] x86/fpu/xstate: Warn if guest-only supervisor
 states are detected in normal fpstate

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