linux-kernel - Re: [PATCH] x86/fpu/xstate: Fix an xstate size check warning

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:   Mon, 20 Jul 2020 20:33:13 +0300
From:   Cyrill Gorcunov <gorcunov@...il.com>
To:     kan.liang@...ux.intel.com
Cc:     peterz@...radead.org, mingo@...hat.com, acme@...nel.org,
        tglx@...utronix.de, bp@...en8.de, x86@...nel.org,
        linux-kernel@...r.kernel.org, dave.hansen@...el.com,
        yu-cheng.yu@...el.com, bigeasy@...utronix.de, hpa@...or.com,
        eranian@...gle.com, ak@...ux.intel.com, chang.seok.bae@...el.com
Subject: Re: [PATCH] x86/fpu/xstate: Fix an xstate size check warning

On Mon, Jul 20, 2020 at 06:50:51AM -0700, kan.liang@...ux.intel.com wrote:
...
>  static unsigned int __init get_xsave_size(void)
>  {
>  	unsigned int eax, ebx, ecx, edx;
> @@ -710,7 +741,7 @@ static int __init init_xstate_size(void)
>  	xsave_size = get_xsave_size();
>  
>  	if (boot_cpu_has(X86_FEATURE_XSAVES))
> -		possible_xstate_size = get_xsaves_size();
> +		possible_xstate_size = get_xsaves_size_no_dynamic();
>  	else
>  		possible_xstate_size = xsave_size;

Hi! Maybe we could enhance get_xsaves_size instead ? The get_xsaves_size is
static and __init function (thus not a hot path) used once as far as I see.
Say

static unsigned int __init get_xsaves_size(void)
{
	u64 mask = xfeatures_mask_dynamic();
	unsigned int eax, ebx, ecx, edx;

	/*
	 * In case if dynamic features are present make
	 * sure they are not accounted in the result since
	 * the buffer should be allocated separately from
	 * task->fpu.
	 */
	if (mask)
		wrmsrl(MSR_IA32_XSS, xfeatures_mask_supervisor());

	/*
	 * - CPUID function 0DH, sub-function 1:
	 *    EBX enumerates the size (in bytes) required by
	 *    the XSAVES instruction for an XSAVE area
	 *    containing all the state components
	 *    corresponding to bits currently set in
	 *    XCR0 | IA32_XSS.
	 */
	cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx);

	if (mask)
		wrmsrl(MSR_IA32_XSS, xfeatures_mask_supervisor() | mask);

	return ebx;
}

but if you expect more use of get_xsaves_size_no_dynamic() and
get_xsaves_size() in future then sure, we need a separate function.

The benefit from such extension is that when you read get_xsaves_size
you'll notice the dependency on dynamic features immediaely.

Though I'm fine with current patch as well, up to you. Thanks for the patch!

Reviewed-by: Cyrill Gorcunov <gorcunov@...il.com>