linux-kernel - Re: [PATCH v5 5/7] x86/fpu: Initialize guest fpstate and FPU pseudo container from guest defaults

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: <9ca17e1169805f35168eb722734fbf3579187886.camel@intel.com>
Date: Wed, 30 Apr 2025 18:29:55 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "Gao, Chao" <chao.gao@...el.com>, "Hansen, Dave" <dave.hansen@...el.com>,
	"seanjc@...gle.com" <seanjc@...gle.com>, "x86@...nel.org" <x86@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"tglx@...utronix.de" <tglx@...utronix.de>, "kvm@...r.kernel.org"
	<kvm@...r.kernel.org>, "pbonzini@...hat.com" <pbonzini@...hat.com>
CC: "Yang, Weijiang" <weijiang.yang@...el.com>, "Li, Xin3"
	<xin3.li@...el.com>, "ebiggers@...gle.com" <ebiggers@...gle.com>,
	"bp@...en8.de" <bp@...en8.de>, "dave.hansen@...ux.intel.com"
	<dave.hansen@...ux.intel.com>, "peterz@...radead.org" <peterz@...radead.org>,
	"john.allen@....com" <john.allen@....com>, "Bae, Chang Seok"
	<chang.seok.bae@...el.com>, "mingo@...hat.com" <mingo@...hat.com>,
	"hpa@...or.com" <hpa@...or.com>, "Spassov, Stanislav" <stanspas@...zon.de>
Subject: Re: [PATCH v5 5/7] x86/fpu: Initialize guest fpstate and FPU pseudo
 container from guest defaults

On Thu, 2025-04-10 at 15:24 +0800, Chao Gao wrote:
> fpu_alloc_guest_fpstate() currently uses host defaults to initialize guest
> fpstate and pseudo containers. Guest defaults were introduced to
> differentiate the features and sizes of host and guest FPUs. Switch to
> using guest defaults instead.
> 
> Additionally, incorporate the initialization of indicators (is_valloc and
> is_guest) into the newly added guest-specific reset function to centralize
> the resetting of guest fpstate.
> 
> Suggested-by: Chang S. Bae <chang.seok.bae@...el.com>
> Signed-off-by: Chao Gao <chao.gao@...el.com>
> ---
> v5: init is_valloc/is_guest in the guest-specific reset function (Chang)
> ---
>  arch/x86/kernel/fpu/core.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index e23e435b85c4..f5593f6009a4 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -201,7 +201,20 @@ void fpu_reset_from_exception_fixup(void)
>  }
>  
>  #if IS_ENABLED(CONFIG_KVM)
> -static void __fpstate_reset(struct fpstate *fpstate, u64 xfd);
> +static void __guest_fpstate_reset(struct fpstate *fpstate, u64 xfd)
> +{
> +	/* Initialize sizes and feature masks */
> +	fpstate->size		= guest_default_cfg.size;
> +	fpstate->user_size	= guest_default_cfg.user_size;
> +	fpstate->xfeatures	= guest_default_cfg.features;
> +	fpstate->user_xfeatures	= guest_default_cfg.user_features;
> +	fpstate->xfd		= xfd;
> +
> +	/* Initialize indicators to reflect properties of the fpstate */
> +	fpstate->is_valloc	= true;
> +	fpstate->is_guest	= true;
> +}
> +
>  
>  static void fpu_lock_guest_permissions(void)
>  {
> @@ -226,19 +239,18 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu)
>  	struct fpstate *fpstate;
>  	unsigned int size;
>  
> -	size = fpu_kernel_cfg.default_size + ALIGN(offsetof(struct fpstate, regs), 64);
> +	size = guest_default_cfg.size + ALIGN(offsetof(struct fpstate, regs), 64);
> +
>  	fpstate = vzalloc(size);
>  	if (!fpstate)
>  		return false;
>  
>  	/* Leave xfd to 0 (the reset value defined by spec) */
> -	__fpstate_reset(fpstate, 0);
> +	__guest_fpstate_reset(fpstate, 0);
>  	fpstate_init_user(fpstate);
> -	fpstate->is_valloc	= true;
> -	fpstate->is_guest	= true;
>  
>  	gfpu->fpstate		= fpstate;
> -	gfpu->xfeatures		= fpu_kernel_cfg.default_features;
> +	gfpu->xfeatures		= guest_default_cfg.features;
>  
>  	/*
>  	 * KVM sets the FP+SSE bits in the XSAVE header when copying FPU state
> @@ -250,8 +262,8 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu)
>  	 * all features that can expand the uABI size must be opt-in.
>  	 */

The above comment is enlightening to the debate about whether guest needs a
separate user size and features:

	/*
	 * KVM sets the FP+SSE bits in the XSAVE header when copying FPU state
	 * to userspace, even when XSAVE is unsupported, so that restoring FPU
	 * state on a different CPU that does support XSAVE can cleanly load
	 * the incoming state using its natural XSAVE.  In other words, KVM's
	 * uABI size may be larger than this host's default size.  Conversely,
	 * the default size should never be larger than KVM's base uABI size;
	 * all features that can expand the uABI size must be opt-in.
	 */


The KVM FPU user xsave behavior *is* different, just not in the way than we have
been discussing. So the below code responds to mismatch between
fpu_user_cfg.default_size and KVM's ABI.

The fix that added it, d187ba531230 ("x86/fpu: KVM: Set the base guest FPU uABI
size to sizeof(struct kvm_xsave)"), seems like quick fix that could have instead
been fixed more properly by something like proposed in this series.

I propose we drop it from this series and follow up with a proper cleanup. It
deserves more than currently done here. For example in the below hunk it's now
comparing guest_default_cfg.user_size which is a guest only thing. I also wonder
if we really need gfpu->uabi_size.

So let's drop the code but not the idea. Chang what do you think of that?

>  	gfpu->uabi_size		= sizeof(struct kvm_xsave);
> -	if (WARN_ON_ONCE(fpu_user_cfg.default_size > gfpu->uabi_size))
> -		gfpu->uabi_size = fpu_user_cfg.default_size;
> +	if (WARN_ON_ONCE(guest_default_cfg.user_size > gfpu->uabi_size))
> +		gfpu->uabi_size = guest_default_cfg.user_size;
>  
>  	fpu_lock_guest_permissions();
>