linux-kernel - Re: [PATCH] fpu: xstate: Keep xfd_state always in-sync with IA32

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]

Date:   Thu, 11 May 2023 10:59:08 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Adamos Ttofari <attofari@...zon.de>
Cc:     abusse@...zon.de, dwmw@...zon.co.uk, hborghor@...zon.de,
        sironi@...zon.de, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>, Kyle Huey <me@...ehuey.com>,
        "Chang S. Bae" <chang.seok.bae@...el.com>,
        Andrew Cooper <andrew.cooper3@...rix.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fpu: xstate: Keep xfd_state always in-sync with IA32_XFD
 MSR

On 5/11/23 08:28, Adamos Ttofari wrote:
> @@ -179,8 +179,14 @@ void fpu__init_cpu_xstate(void)
>  	 * key as that does not work on the boot CPU. This also ensures
>  	 * that any stale state is wiped out from XFD.
>  	 */
> -	if (cpu_feature_enabled(X86_FEATURE_XFD))
> -		wrmsrl(MSR_IA32_XFD, init_fpstate.xfd);
> +	if (cpu_feature_enabled(X86_FEATURE_XFD)) {
> +		u64 xfd;
> +
> +		rdmsrl(MSR_IA32_XFD, xfd);
> +		__this_cpu_write(xfd_state, xfd);
> +
> +		xfd_update_state(&init_fpstate);
> +	}

The above comment didn't _quite_ make it into the context, so I'll paste
it here for your convenience:

>         /*
>          * Must happen after CR4 setup and before xsetbv() to allow KVM
>          * lazy passthrough.  Write independent of the dynamic state static
>          * key as that does not work on the boot CPU. This also ensures
>          * that any stale state is wiped out from XFD.
>          */

Translating there, "the dynamic state static key" means
'__fpu_state_size_dynamic' which is used here:

> static __always_inline __pure bool fpu_state_size_dynamic(void)
> {
>         return static_branch_unlikely(&__fpu_state_size_dynamic);
> }

You might recognize fpu_state_size_dynamic() from the first line of
xfd_update_state(), the function that you added to the above hunk.

Which brings me to ask what the point of calling xfd_update_state() is
in the first place if you're getting away with it not working on the
boot CPU.

Why not just short-circuit the (non-working) xfd_update_state() and do
this directly:

	wrmsrl(MSR_IA32_XFD, init_fpstate.xfd);
        __this_cpu_write(xfd_state, init_fpstate.xfd);

I don't think you even need to *READ* the MSR.  You're going to blow it
away anyway.

>  	/*
>  	 * XCR_XFEATURE_ENABLED_MASK (aka. XCR0) sets user features
> @@ -915,7 +921,7 @@ void fpu__resume_cpu(void)
>  	}
>  
>  	if (fpu_state_size_dynamic())
> -		wrmsrl(MSR_IA32_XFD, current->thread.fpu.fpstate->xfd);
> +		xfd_update_state(&init_fpstate);
>  }
>  
>  /*