linux-kernel - Re: Should Linux set the new constant-time mode CPU flags?

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: <Y1du6kdk/6L0O56a@sol.localdomain>
Date:   Mon, 24 Oct 2022 22:06:50 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Dave Hansen <dave.hansen@...el.com>,
        "Jason A. Donenfeld" <Jason@...c4.com>, x86@...nel.org,
        linux-arm-kernel@...ts.infradead.org, linux-crypto@...r.kernel.org,
        linux-kernel@...r.kernel.org, Adam Langley <agl@...gle.com>,
        Ard Biesheuvel <ardb@...nel.org>
Subject: Re: Should Linux set the new constant-time mode CPU flags?

On Thu, Sep 01, 2022 at 01:00:29PM +0200, Peter Zijlstra wrote:
> Since I'm not feeling too well I figured I'd do something trivial and
> whipped up the below patch.
> 
> 
> ---
>  arch/x86/include/asm/cpufeatures.h |  3 ++
>  arch/x86/include/asm/msr-index.h   |  4 +++
>  arch/x86/kernel/cpu/common.c       | 69 ++++++++++++++++++++++++++++++--------
>  arch/x86/kernel/cpu/scattered.c    |  1 +
>  4 files changed, 63 insertions(+), 14 deletions(-)

We still need to do something about this.  As this thread died out, I'll revive
it by reviewing this patch.  (I'm not an expert in arch/x86/ stuff, though!)

> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index 333d94394516..9b92f4e5e80a 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -305,6 +305,7 @@
>  #define X86_FEATURE_USE_IBPB_FW		(11*32+16) /* "" Use IBPB during runtime firmware calls */
>  #define X86_FEATURE_RSB_VMEXIT_LITE	(11*32+17) /* "" Fill RSB on VM exit when EIBRS is enabled */
>  #define X86_FEATURE_CALL_DEPTH		(11*32+18) /* "" Call depth tracking for RSB stuffing */
> +#define X86_FEATURE_MCDT_NO		(11*32+19) /* Not affected by MCDT */

Some of the other CPU feature flags have comments beginning with "", which
apparently results in the feature not being listed in /proc/cpuinfo.  (This
header file is run through a shell script that looks at these comments and
generates C code...)  Should this "feature" be listed in /proc/cpuinfo?

Looking for examples of other "feature" flags that mean that a CPU is not
vulnerable to something, I found X86_FEATURE_AMD_SSB_NO and X86_FEATURE_BTC_NO.
Those aren't listed in /proc/cpuinfo.  Maybe this should be the same?

Side note: maybe the comment should spell out "MXCSR Configuration Dependent
Timing"?  Acronyms can be hard to read.

> +#define X86_BUG_DOIT			X86_BUG(28)

Maybe it should be X86_BUG_DODT?  The bug is data operand *dependent* timing.
Data operand *independent* timing is the desired behavior and the fix.

> +#define X86_BUG_MCDT			X86_BUG(29)

According to Intel's documentation
(https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/best-practices/data-operand-independent-timing-isa-guidance.html),
MCDT is a separate bug which requires a separate mitigation.  So I think any
MCDT related stuff should be in a separate patch from DOITM.

But more importantly, this patch doesn't actually implement any mitigation for
MCDT.  Should we be doing that?  Intel recommends writing a certain value to
MXCSR to mitigate MCDT.  Is that feasible?

>  
>  #endif /* _ASM_X86_CPUFEATURES_H */
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 6674bdb096f3..08b4e0c2f7d3 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -119,6 +119,7 @@
>  						 * Not susceptible to
>  						 * TSX Async Abort (TAA) vulnerabilities.
>  						 */
> +#define ARCH_CAP_DOIT			BIT(12) /* Data Operand Independent Timing */
>  #define ARCH_CAP_SBDR_SSDP_NO		BIT(13)	/*
>  						 * Not susceptible to SBDR and SSDP
>  						 * variants of Processor MMIO stale data
> @@ -155,6 +156,9 @@
>  						 * Return Stack Buffer Predictions.
>  						 */
>  
> +#define MSR_IA32_UARCH_MISC_CTL		0x00001b01
> +#define UARCH_MISC_DOIT			BIT(0)	/* Enable DOIT */

The Intel documentation calls this bit "DOITM"
(Data Operand Independent Timing Mode), not "DOIT".

> +static __always_inline void setup_doit(struct cpuinfo_x86 *c)
> +{
> +	u64 msr = 0;
> +
> +	if (!cpu_has(c, X86_BUG_DOIT))
> +		return;
> +
> +	if (!doit_disabled)
> +		return;

This is backwards; it needs to be 'if (doit_disabled)'.

> diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
> index fd44b54c90d5..5063f8046554 100644
> --- a/arch/x86/kernel/cpu/scattered.c
> +++ b/arch/x86/kernel/cpu/scattered.c
> @@ -27,6 +27,7 @@ static const struct cpuid_bit cpuid_bits[] = {
>  	{ X86_FEATURE_APERFMPERF,       CPUID_ECX,  0, 0x00000006, 0 },
>  	{ X86_FEATURE_EPB,		CPUID_ECX,  3, 0x00000006, 0 },
>  	{ X86_FEATURE_INTEL_PPIN,	CPUID_EBX,  0, 0x00000007, 1 },
> +	{ X86_FEATURE_MCDT_NO,		CPUID_ECX,  5, 0x00000007, 2 },

The Intel documentation says this bit is CPUID.(EAX=7H,ECX=2):EDX[5]=1.
So CPUID_ECX is wrong; it needs to be CPUID_EDX.

- Eric