linux-kernel - Re: [RFC PATCH 2/8] x86/fpu/xstate: Fix guest fpstate allocation size calculation

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: <ZTMbfUhyOkmA9czp@google.com>
Date:   Fri, 20 Oct 2023 17:29:49 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     Yang Weijiang <weijiang.yang@...el.com>
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org,
        dave.hansen@...el.com, tglx@...utronix.de, peterz@...radead.org,
        pbonzini@...hat.com, rick.p.edgecombe@...el.com,
        kvm@...r.kernel.org, yang.zhong@...el.com, jing2.liu@...el.com,
        chao.gao@...el.com
Subject: Re: [RFC PATCH 2/8] x86/fpu/xstate: Fix guest fpstate allocation size calculation

On Wed, Sep 13, 2023, Yang Weijiang wrote:
> Fix guest xsave area allocation size from fpu_user_cfg.default_size to
> fpu_kernel_cfg.default_size so that the xsave area size is consistent
> with fpstate->size set in __fpstate_reset().
> 
> With the fix, guest fpstate size is sufficient for KVM supported guest
> xfeatures.
> 
> Fixes: 69f6ed1d14c6 ("x86/fpu: Provide infrastructure for KVM FPU cleanup");
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Signed-off-by: Yang Weijiang <weijiang.yang@...el.com>
> ---
>  arch/x86/kernel/fpu/core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index a86d37052a64..a42d8ad26ce6 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -220,7 +220,9 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu)
>  	struct fpstate *fpstate;
>  	unsigned int size;
>  
> -	size = fpu_user_cfg.default_size + ALIGN(offsetof(struct fpstate, regs), 64);
> +	size = fpu_kernel_cfg.default_size +
> +	       ALIGN(offsetof(struct fpstate, regs), 64);

This looks sketchy and incomplete.  I haven't looked at the gory details of
fpu_user_cfg vs. fpu_kernel_cfg, but the rest of this function uses fpu_user_cfg,
including a check on fpu_user_cfg.default_size.  That makes me think that changing
just the allocation size isn't quite right.

	/* 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_user_cfg.default_features;
	gfpu->perm		= fpu_user_cfg.default_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.
	 */
	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;