| 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
| ||
|
Message-ID: <aBpFvyITMc0WhlX4@google.com>
Date: Tue, 6 May 2025 10:24:15 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Chao Gao <chao.gao@...el.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
On Tue, May 06, 2025, 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>
> ---
> v6: Drop vcpu_fpu_config.user_* (Rick)
> v5: init is_valloc/is_guest in the guest-specific reset function (Chang)
> ---
> arch/x86/kernel/fpu/core.c | 28 ++++++++++++++++++++++------
> 1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index 444e517a8648..78a9c809dfad 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -211,7 +211,24 @@ 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. Supervisor features and
> + * sizes may diverge between guest FPUs and host FPUs, whereas
> + * user features and sizes remain the same.
> + */
> + fpstate->size = guest_default_cfg.size;
> + fpstate->xfeatures = guest_default_cfg.features;
> + fpstate->user_size = fpu_user_cfg.default_size;
> + fpstate->user_xfeatures = fpu_user_cfg.default_features;
> + fpstate->xfd = xfd;
> +
> + /* Initialize indicators to reflect properties of the fpstate */
> + fpstate->is_valloc = true;
> + fpstate->is_guest = true;
> +}
> +
Extra newline.
>
> 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.
Passing in '0' and setting xfd in __guest_fpstate_reset() is especially pointless.
E.g.
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index d5f3af2ba758..87d6ee87ff55 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -212,25 +212,6 @@ void fpu_reset_from_exception_fixup(void)
}
#if IS_ENABLED(CONFIG_KVM)
-static void __guest_fpstate_reset(struct fpstate *fpstate, u64 xfd)
-{
- /*
- * Initialize sizes and feature masks. Supervisor features and
- * sizes may diverge between guest FPUs and host FPUs, whereas
- * user features and sizes remain the same.
- */
- fpstate->size = guest_default_cfg.size;
- fpstate->xfeatures = guest_default_cfg.features;
- fpstate->user_size = fpu_user_cfg.default_size;
- fpstate->user_xfeatures = fpu_user_cfg.default_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)
{
struct fpu_state_perm *fpuperm;
@@ -260,8 +241,20 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu)
if (!fpstate)
return false;
- /* Leave xfd to 0 (the reset value defined by spec) */
- __guest_fpstate_reset(fpstate, 0);
+ /* Initialize indicators to reflect properties of the fpstate */
+ fpstate->is_valloc = true;
+ fpstate->is_guest = true;
+
+ /*
+ * 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.
+ */
+ fpstate->size = guest_default_cfg.size;
+ fpstate->xfeatures = guest_default_cfg.features;
+ fpstate->user_size = fpu_user_cfg.default_size;
+ fpstate->user_xfeatures = fpu_user_cfg.default_features;
+
fpstate_init_user(fpstate);
gfpu->fpstate = fpstate;
@@ -550,21 +543,17 @@ void fpstate_init_user(struct fpstate *fpstate)
fpstate_init_fstate(fpstate);
}
-static void __fpstate_reset(struct fpstate *fpstate, u64 xfd)
-{
- /* Initialize sizes and feature masks */
- fpstate->size = fpu_kernel_cfg.default_size;
- 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;
-}
-
void fpstate_reset(struct fpu *fpu)
{
/* Set the fpstate pointer to the default fpstate */
fpu->fpstate = &fpu->__fpstate;
- __fpstate_reset(fpu->fpstate, init_fpstate.xfd);
+
+ /* Initialize sizes and feature masks */
+ fpu->fpstate->size = fpu_kernel_cfg.default_size;
+ fpu->fpstate->user_size = fpu_user_cfg.default_size;
+ fpu->fpstate->xfeatures = fpu_kernel_cfg.default_features;
+ fpu->fpstate->user_xfeatures = fpu_user_cfg.default_features;
+ fpu->fpstate->xfd = init_fpstate.xfd;
/* Initialize the permission related info in fpu */
fpu->perm.__state_perm = fpu_kernel_cfg.default_features;
Powered by blists - more mailing lists