linux-kernel - Re: [RFC PATCH] x86/fpu/xstate: Add more diagnostic information on inconsistent xstate sizes

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:   Fri, 7 Apr 2023 11:22:37 -0700
From:   "Chang S. Bae" <chang.seok.bae@...el.com>
To:     Fenghua Yu <fenghua.yu@...el.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Borislav Petkov <bp@...en8.de>, Ingo Molnar <mingo@...hat.com>
CC:     linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>,
        Chintan M Patel <chintan.m.patel@...el.com>,
        Thiago Macieira <thiago.macieira@...el.com>
Subject: Re: [RFC PATCH] x86/fpu/xstate: Add more diagnostic information on
 inconsistent xstate sizes

On 4/5/2023 11:39 AM, Fenghua Yu wrote:
> A warning is emitted when xstate sizes are found inconsistent.
> But the warning message doesn't show enough information to diagnose
> the issue.
> 
> Provide more detailed xstate size information to help debug the issue.
> As a hypothetical example, on a platform that may report incorrect
> xstate size in CPUID.0xd.1:EBX, the diagnostic information after
> the warning will show:
> [    0.000000] x86/fpu: max_features=0x407
> [    0.000000] x86/fpu: xstate_offset[2]:  576, xstate_sizes[2]:  256
> [    0.000000] x86/fpu: xstate_offset[10]: 832, xstate_sizes[10]:    8
> [    0.000000] x86/fpu: total size: 840 bytes
> [    0.000000] x86/fpu: XCR0=0x7, IA32_XSS=0x400
> [    0.000000] x86/fpu: kernel_size from CPUID.0xd.0x1:EBX: 576 bytes
> 
> XCR0 | IA32_XSS is 0x407 which is consistent with max_features.
> CPUID.0xd.0x1:EBX should report the size of the xsave area
> containing xstate components corresponding to bits set in
> XCR0 | IA32_XSS. But it only reports 576 bytes of xsave area which
> doesn't count xstate sizes for AVX (offset 2 and 256 bytes) and
> PASID (offset 10 and 8 bytes). This confirms that the platform
> reports xstate size incorrectly through the CPUID bits.
> 
> Suggest-by: Dave Hansen <dave.hansen@...ux.intel.com>
> Tested-by: Chintan M Patel <chintan.m.patel@...el.com>
> Signed-off-by: Fenghua Yu <fenghua.yu@...el.com>
> ---
>   arch/x86/kernel/fpu/xstate.c | 33 +++++++++++++++++++++++++++++++--
>   1 file changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 0bab497c9436..5f27fcdc6c90 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -602,8 +602,37 @@ static bool __init paranoid_xstate_size_valid(unsigned int kernel_size)
>   		}
>   	}
>   	size = xstate_calculate_size(fpu_kernel_cfg.max_features, compacted);
> -	XSTATE_WARN_ON(size != kernel_size,
> -		       "size %u != kernel_size %u\n", size, kernel_size);
> +	if (size != kernel_size) {
> +		u64 xcr0, ia32_xss;
> +
> +		XSTATE_WARN_ON(1, "size %u != kernel_size %u\n",
> +			       size, kernel_size);
> +
> +		/* Show more information to help diagnose the size issue. */
> +		pr_info("x86/fpu: max_features=0x%llx\n",
> +			fpu_kernel_cfg.max_features);
> +		print_xstate_offset_size();
> +		pr_info("x86/fpu: total size: %u bytes\n", size);
> +		xcr0 = xgetbv(XCR_XFEATURE_ENABLED_MASK);
> +		if (compacted) {
> +			rdmsrl(MSR_IA32_XSS, ia32_xss);

This shouldn't be directly read here because of the LBR state component.

See the function comment:

  * Independent XSAVE features allocate their own buffers and are not
  * covered by these checks. Only the size of the buffer for task->fpu
  * is checked here.

But, isn't that max_features bitmask pretty much about it?

> +			pr_info("x86/fpu: XCR0=0x%llx, IA32_XSS=0x%llx\n",
> +				xcr0, ia32_xss);
> +		} else {
> +			pr_info("x86/fpu: XCR0=0x%llx\n", xcr0);
> +		}
> +		/*
> +		 * In compact case, CPUID.0xd.0x1:EBX reports the size of
> +		 * the XSAVE size containing all the state components
> +		 * corresponding to bits set in XCR0 | IA32_XSS.
> +		 *
> +		 * Otherwise, CPUID.0xd.0x0:EBX reports the size of an XSAVE
> +		 * area containing all the *user* state components
> +		 * corresponding to bits set in XCR0.
> +		 */
> +		pr_info("x86/fpu: kernel_size from CPUID.0xd.0x%x:EBX: %u bytes\n",
> +			compacted ? 1 : 0, kernel_size);

Include Thiago who asked this.

Thanks,
Chang