lkml.org 
[lkml]   [2022]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: 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

\
 
 \ /
  Last update: 2022-10-25 07:08    [W:0.062 / U:26.628 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site