linux-kernel - Re: [PATCH v6 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: <aBsNxmHE7UO03iCs@intel.com>
Date: Wed, 7 May 2025 15:37:42 +0800
From: Chao Gao <chao.gao@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
CC: <x86@...nel.org>, <linux-kernel@...r.kernel.org>, <kvm@...r.kernel.org>,
	<tglx@...utronix.de>, <dave.hansen@...el.com>, <pbonzini@...hat.com>,
	<peterz@...radead.org>, <rick.p.edgecombe@...el.com>,
	<weijiang.yang@...el.com>, <john.allen@....com>, <bp@...en8.de>,
	<chang.seok.bae@...el.com>, <xin3.li@...el.com>, Ingo Molnar
	<mingo@...hat.com>, Dave Hansen <dave.hansen@...ux.intel.com>, "H. Peter
 Anvin" <hpa@...or.com>, Oleg Nesterov <oleg@...hat.com>, Eric Biggers
	<ebiggers@...gle.com>, Stanislav Spassov <stanspas@...zon.de>, Kees Cook
	<kees@...nel.org>
Subject: Re: [PATCH v6 5/7] x86/fpu: Initialize guest fpstate and FPU pseudo
 container from guest defaults

>>  static void fpu_lock_guest_permissions(void)
>>  {
>> @@ -236,19 +253,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);
>
>Given that there is a single caller for each of __fpstate_reset() and
>__guest_fpstate_reset(), keeping the helpers does more harm than good IMO.

Seems Dave strongly dislikes inlining the helpers

https://lore.kernel.org/kvm/98dda94f-b805-4e0e-871a-085eb2f6ff20@intel.com/

>Passing in '0' and setting xfd in __guest_fpstate_reset() is especially pointless.

Agreed.

Giving Dave's feedback, how about tweaking __fpstate_reset() for guest FPUs:

>From 13eddc8a0ecbd9e2a59dc1caacc7dbf198277a24 Mon Sep 17 00:00:00 2001
From: Chao Gao <chao.gao@...el.com>
Date: Fri, 31 May 2024 02:03:30 -0700
Subject: [PATCH] x86/fpu: Initialize guest fpstate and FPU pseudo container
 from guest defaults

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.

Adjust __fpstate_reset() to handle different defaults for host and guest
FPUs. And to distinguish between the types of FPUs, move the initialization
of indicators (is_guest and is_valloc) before the reset.

Suggested-by: Chang S. Bae <chang.seok.bae@...el.com>
Signed-off-by: Chao Gao <chao.gao@...el.com>
---
 arch/x86/kernel/fpu/core.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 444e517a8648..0d501bd25d79 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -236,19 +236,22 @@ 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;
 
+	/* Initialize indicators to reflect properties of the fpstate */
+	fpstate->is_valloc	= true;
+	fpstate->is_guest	= true;
+
 	/* Leave xfd to 0 (the reset value defined by spec) */
 	__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
@@ -535,10 +538,20 @@ void fpstate_init_user(struct fpstate *fpstate)
 
 static void __fpstate_reset(struct fpstate *fpstate, u64 xfd)
 {
-	/* Initialize sizes and feature masks */
-	fpstate->size		= fpu_kernel_cfg.default_size;
+	/*
+	 * Initialize sizes and feature masks. Supervisor features and
+	 * sizes may diverge between guest FPUs and host FPUs, whereas
+	 * user features and sizes are always identical the same.
+	 */
+	if (fpstate->is_guest) {
+		fpstate->size		= guest_default_cfg.size;
+		fpstate->xfeatures	= guest_default_cfg.features;
+	} else {
+		fpstate->size		= fpu_kernel_cfg.default_size;
+		fpstate->xfeatures	= fpu_kernel_cfg.default_features;
+	}
+
 	fpstate->user_size	= fpu_user_cfg.default_size;
-	fpstate->xfeatures	= fpu_kernel_cfg.default_features;
 	fpstate->user_xfeatures	= fpu_user_cfg.default_features;
 	fpstate->xfd		= xfd;
 }
-- 
2.47.1