Should Linux set the new constant-time mode CPU flags?

Should Linux set the new constant-time mode CPU flags?

[Eric Biggers]

Hi,

Intel and ARM recently published documentation that says that no instructions
are guaranteed to be constant-time with respect to their data operands, unless a
"data independent timing" flag in the IA32_UARCH_MISC_CTL register (Intel) or
DIT register (arm64) is set:

* https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/best-practices/data-operand-independent-timing-isa-guidance.html
* https://developer.arm.com/documentation/ddi0595/2021-06/AArch64-Registers/DIT--Data-Independent-Timing

This is a major problem for crypto code, which needs to be constant-time,
especially with respect to keys.  And since this is a CPU issue, it affects all
code running on the CPU.  While neither company is treating this as a security
disclosure, to me this looks exactly like a CPU vulnerability.

For Intel, given that the mitigation is to set an MSR flag, it seems that the
kernel will need to do that -- similar to the MSR flags that enable mitigations
for speculative execution vulnerabilities.

For arm64, it's not clear to me whether the DIT flag is privileged or not.  If
privileged, I expect it would need to be set by the kernel just like the Intel
flag.  If unprivileged, I expect there will still be work to do in the kernel,
as the flag will need to be set when running any crypto code in the kernel.

I'm wondering if people are aware of this issue, and whether anyone has any
thoughts on whether/where the kernel should be setting these new CPU flags.
There don't appear to have been any prior discussions about this.  (Thanks to
Adam Langley, who maintains BoringSSL, for bringing this to my attention.)

- Eric

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

[Peter Zijlstra]

On Thu, Aug 25, 2022 at 11:15:58PM +0000, Eric Biggers wrote:
> Hi,
> 
> Intel and ARM recently published documentation that says that no instructions
> are guaranteed to be constant-time with respect to their data operands, unless a
> "data independent timing" flag in the IA32_UARCH_MISC_CTL register (Intel) or
> DIT register (arm64) is set:
> 
> * https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/best-practices/data-operand-independent-timing-isa-guidance.html
> * https://developer.arm.com/documentation/ddi0595/2021-06/AArch64-Registers/DIT--Data-Independent-Timing
> 
> This is a major problem for crypto code, which needs to be constant-time,
> especially with respect to keys.  And since this is a CPU issue, it affects all
> code running on the CPU.  While neither company is treating this as a security
> disclosure, to me this looks exactly like a CPU vulnerability.
> 
> For Intel, given that the mitigation is to set an MSR flag, it seems that the
> kernel will need to do that -- similar to the MSR flags that enable mitigations
> for speculative execution vulnerabilities.
> 
> For arm64, it's not clear to me whether the DIT flag is privileged or not.  If
> privileged, I expect it would need to be set by the kernel just like the Intel
> flag.  If unprivileged, I expect there will still be work to do in the kernel,
> as the flag will need to be set when running any crypto code in the kernel.
> 
> I'm wondering if people are aware of this issue, and whether anyone has any
> thoughts on whether/where the kernel should be setting these new CPU flags.
> There don't appear to have been any prior discussions about this.  (Thanks to
> Adam Langley, who maintains BoringSSL, for bringing this to my attention.)

Whichever way around I think you want OS support to make it a per-task
property instead of a per CPU one.

Also, *sigh* yet another MSR to touch :/

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

[Arnd Bergmann]

On Fri, Aug 26, 2022 at 1:15 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> For arm64, it's not clear to me whether the DIT flag is privileged or not.  If
> privileged, I expect it would need to be set by the kernel just like the Intel
> flag.  If unprivileged, I expect there will still be work to do in the kernel,
> as the flag will need to be set when running any crypto code in the kernel.

7206dc93a58f ("arm64: Expose Arm v8.4 features") added the feature bit for
Armv8.4+ processors. From what I can tell from the documentation and the
kernel source, I see:

- if the feature is set in HWCAP (or /proc/cpuinfo), then the instruction DIT
  register is available in user space, and sensitive code can set or clear the
  constant-time mode for the local thread.
- On CPUs without the feature (almost all ARMv8 ones), the register should
  not be touched.
- The bit is context switched on kernel entry, so setting the bit in user space
  does not change the behavior inside of a syscall
- If we add a user space interface for setting the bit per thread on x86,
  the same interface could be supported to set the bit on arm64 to save
  user space implementations the trouble of checking the feature bits
- the in-kernel crypto code does not set the bit today but could be easily
  changed to do this for CPUs that support it, if we can decide on a policy
  for when to enable or disable it.

        Arnd

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

[Catalin Marinas]

(catching up with this thread)

On Fri, Aug 26, 2022 at 10:45:07AM +0200, Arnd Bergmann wrote:
> On Fri, Aug 26, 2022 at 1:15 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > For arm64, it's not clear to me whether the DIT flag is privileged or not.  If
> > privileged, I expect it would need to be set by the kernel just like the Intel
> > flag.  If unprivileged, I expect there will still be work to do in the kernel,
> > as the flag will need to be set when running any crypto code in the kernel.
> 
> 7206dc93a58f ("arm64: Expose Arm v8.4 features") added the feature bit for
> Armv8.4+ processors. From what I can tell from the documentation and the
> kernel source, I see:
> 
> - if the feature is set in HWCAP (or /proc/cpuinfo), then the instruction DIT
>   register is available in user space, and sensitive code can set or clear the
>   constant-time mode for the local thread.

Indeed, the arm64 DIT feature can be enabled in user space, subject to
checking the HWCAP bit or the CPUID regs (via kernel trapping and
emulation). The expectation was that some crypto routines would set it
on function entry, restore it on return but...

> - On CPUs without the feature (almost all ARMv8 ones), the register should
>   not betouched.

That's one of the drawbacks of using the features in user-space (the
instruction is not in the hint/nop space). It can be worked around with
ifunc resolvers but with a slight overhead on function calling.

> - The bit is context switched on kernel entry, so setting the bit in user space
>   does not change the behavior inside of a syscall
> - If we add a user space interface for setting the bit per thread on x86,
>   the same interface could be supported to set the bit on arm64 to save
>   user space implementations the trouble of checking the feature bits

A prctl() would do here but I think the default should be off or at
least allow a sysctl to control this. Enabling DIT could have a small
performance impact while lots of (most?) apps don't need such
guarantees.

For arm64, my preference is to have this option per-thread and even be
able to toggle it within a thread (not sure that's possible on x86
without a syscall).

Other random ideas of deploying this (for arm64): have an ELF annotation
that data independent timing is required. If that's on the main
executable, the kernel could turn it on for the app. If it's on a
(crypto) library, it's up to the dynamic loader to either turn it on for
the whole app or just use some function veneers to save/restore it when
the library code is executed.

I assume having this per-thread would work on x86 as well but I'm not
sure about the context switching cost.

> - the in-kernel crypto code does not set the bit today but could be easily
>   changed to do this for CPUs that support it, if we can decide on a policy
>   for when to enable or disable it.

In the kernel it's easier, at least for arm64, to enable it for specific
functions (we can do boot-time code patching).

Whichever way we support it, I'd rather not turn it on by default.
Talking to some of the Arm microarchitects, such feature may prevent
certain hardware optimisations.

-- 
Catalin

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

[Jeffrey Walton]

On Thu, Aug 25, 2022 at 7:24 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> Hi,
>
> Intel and ARM recently published documentation that says that no instructions
> are guaranteed to be constant-time with respect to their data operands, unless a
> "data independent timing" flag in the IA32_UARCH_MISC_CTL register (Intel) or
> DIT register (arm64) is set:
>
> * https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/best-practices/data-operand-independent-timing-isa-guidance.html
> * https://developer.arm.com/documentation/ddi0595/2021-06/AArch64-Registers/DIT--Data-Independent-Timing
>
> This is a major problem for crypto code, which needs to be constant-time,
> especially with respect to keys.  And since this is a CPU issue, it affects all
> code running on the CPU.  While neither company is treating this as a security
> disclosure, to me this looks exactly like a CPU vulnerability.
>
> For Intel, given that the mitigation is to set an MSR flag, it seems that the
> kernel will need to do that -- similar to the MSR flags that enable mitigations
> for speculative execution vulnerabilities.
>
> For arm64, it's not clear to me whether the DIT flag is privileged or not.  If
> privileged, I expect it would need to be set by the kernel just like the Intel
> flag.  If unprivileged, I expect there will still be work to do in the kernel,
> as the flag will need to be set when running any crypto code in the kernel.
>
> I'm wondering if people are aware of this issue, and whether anyone has any
> thoughts on whether/where the kernel should be setting these new CPU flags.
> There don't appear to have been any prior discussions about this.  (Thanks to
> Adam Langley, who maintains BoringSSL, for bringing this to my attention.)

The thought that comes to mind for me is, please make setting/clearing
the bit available in userland. Libraries like Botan, Crypto++ and
OpenSSL could benefit from it.

Jeff

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

[Jason A. Donenfeld]

Hi Eric,

On Thu, Aug 25, 2022 at 11:15:58PM +0000, Eric Biggers wrote:
> I'm wondering if people are aware of this issue, and whether anyone has any
> thoughts on whether/where the kernel should be setting these new CPU flags.
> There don't appear to have been any prior discussions about this.  (Thanks to

Maybe it should be set unconditionally now, until we figure out how to
make it more granular.

In terms of granularity, I saw other folks suggesting making it per-task
(so, presumably, a prctl() knob), and others mentioning doing it just
for kernel crypto. For the latter, I guess the crypto API could set it
inside of its abstractions, and the various lib/crypto APIs could set it
at invocation time. I wonder, though, what's the cost of
enabling/disabling it? Would we in fact need a kind of lazy-deferred
disabling, like we have with kernel_fpu_end()? I also wonder what
crypto-adjacent code might wind up being missed if we're going function
by function. Like, obviously we'd set this for crypto_memneq, but what
about potential unprotected `==` of ID numbers that could leak some info
in various protocols? What other subtle nearby code should we be
thinking about, that relies on constant time logic but isn't neatly
folded inside a crypto_do_something() function?

Jason

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

[Eric Biggers]

On Mon, Aug 29, 2022 at 12:39:53PM -0400, Jason A. Donenfeld wrote:
> Hi Eric,
> 
> On Thu, Aug 25, 2022 at 11:15:58PM +0000, Eric Biggers wrote:
> > I'm wondering if people are aware of this issue, and whether anyone has any
> > thoughts on whether/where the kernel should be setting these new CPU flags.
> > There don't appear to have been any prior discussions about this.  (Thanks to
> 
> Maybe it should be set unconditionally now, until we figure out how to
> make it more granular.
> 
> In terms of granularity, I saw other folks suggesting making it per-task
> (so, presumably, a prctl() knob), and others mentioning doing it just
> for kernel crypto. For the latter, I guess the crypto API could set it
> inside of its abstractions, and the various lib/crypto APIs could set it
> at invocation time. I wonder, though, what's the cost of
> enabling/disabling it? Would we in fact need a kind of lazy-deferred
> disabling, like we have with kernel_fpu_end()? I also wonder what
> crypto-adjacent code might wind up being missed if we're going function
> by function. Like, obviously we'd set this for crypto_memneq, but what
> about potential unprotected `==` of ID numbers that could leak some info
> in various protocols? What other subtle nearby code should we be
> thinking about, that relies on constant time logic but isn't neatly
> folded inside a crypto_do_something() function?
> 

I'd much prefer it being set unconditionally by default as well, as making
everyone (both kernel and userspace) turn it on and off constantly would be a
nightmare.

Note that Intel's documentation says that CPUs before Ice Lake behave as if
DOITM is always set:

    "For Intel® Core™ family processors based on microarchitectures before Ice
    Lake and Intel Atom® family processors based on microarchitectures before
    Gracemont that do not enumerate IA32_UARCH_MISC_CTL, developers may assume
    that the instructions listed here operate as if DOITM is enabled."

(It's a bit ambiguous, as it leaves the door open to IA32_UARCH_MISC_CTL being
retroactively added to old CPUs.  But I assume that hasn't actually happened.)

So I think the logical approach is to unconditionally set DOITM by default, to
fix this CPU bug in Ice Lake and later and just bring things back to the way
they were in CPUs before Ice Lake.  With that as a baseline, we can then discuss
whether it's useful to provide ways to re-enable this CPU bug / "feature", for
people who want to get the performance boost (if one actually exists) of data
dependent timing after carefully assessing the risks.

The other way around, of making everything insecure by default, seems like a
really bad idea.

- Eric

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

[Jason A. Donenfeld]

On Mon, Aug 29, 2022 at 06:08:07PM +0000, Eric Biggers wrote:
> I'd much prefer it being set unconditionally by default as well, as making
> everyone (both kernel and userspace) turn it on and off constantly would be a
> nightmare.
> 
> Note that Intel's documentation says that CPUs before Ice Lake behave as if
> DOITM is always set:
> 
>     "For Intel® Core™ family processors based on microarchitectures before Ice
>     Lake and Intel Atom® family processors based on microarchitectures before
>     Gracemont that do not enumerate IA32_UARCH_MISC_CTL, developers may assume
>     that the instructions listed here operate as if DOITM is enabled."
> 
> (It's a bit ambiguous, as it leaves the door open to IA32_UARCH_MISC_CTL being
> retroactively added to old CPUs.  But I assume that hasn't actually happened.)
> 
> So I think the logical approach is to unconditionally set DOITM by default, to
> fix this CPU bug in Ice Lake and later and just bring things back to the way
> they were in CPUs before Ice Lake.  With that as a baseline, we can then discuss
> whether it's useful to provide ways to re-enable this CPU bug / "feature", for
> people who want to get the performance boost (if one actually exists) of data
> dependent timing after carefully assessing the risks.
> 
> The other way around, of making everything insecure by default, seems like a
> really bad idea.

Right. It's actually kind of surprising that Intel didn't already do
this by default. Sure, maybe the Intel manual never explicitly
guaranteed constant time, but a heck of a lot of code relies on that
being the case.

Jason

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

[Jason A. Donenfeld]

On Mon, Aug 29, 2022 at 12:39:53PM -0400, Jason A. Donenfeld wrote:
> In terms of granularity, I saw other folks suggesting making it per-task
> (so, presumably, a prctl() knob), and others mentioning doing it just
> for kernel crypto. For the latter, I guess the crypto API could set it
> inside of its abstractions, and the various lib/crypto APIs could set it
> at invocation time. I wonder, though, what's the cost of
> enabling/disabling it? Would we in fact need a kind of lazy-deferred
> disabling, like we have with kernel_fpu_end()? I also wonder what
> crypto-adjacent code might wind up being missed if we're going function
> by function. Like, obviously we'd set this for crypto_memneq, but what
> about potential unprotected `==` of ID numbers that could leak some info
> in various protocols? What other subtle nearby code should we be
> thinking about, that relies on constant time logic but isn't neatly
> folded inside a crypto_do_something() function?

Another random note on this: I would hope that setting that MSR
represents a speculation barrier or general instruction stream barrier,
so that you can't do something naughty with the scheduler to toggle it
rapidly and measure crypto timings somehow.

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

[Dave Hansen]

On 8/29/22 09:39, Jason A. Donenfeld wrote:
> On Thu, Aug 25, 2022 at 11:15:58PM +0000, Eric Biggers wrote:
>> I'm wondering if people are aware of this issue, and whether anyone has any
>> thoughts on whether/where the kernel should be setting these new CPU flags.
>> There don't appear to have been any prior discussions about this.  (Thanks to
> Maybe it should be set unconditionally now, until we figure out how to
> make it more granular.

Personally, I'm in this camp as well.  Let's be safe and set it by
default.  There's also this tidbit in the Intel docs (and chopping out a
bunch of the noise):

	(On) processors based on microarchitectures before Ice Lake ...
	the instructions listed here operate as if DOITM is enabled.

IOW, setting DOITM=0 isn't going back to the stone age.  At worst, I'd
guess that you're giving up some optimization that only shows up in very
recent CPUs in the first place.

If folks want DOITM=1 on their snazzy new CPUs, then they came come with
performance data to demonstrate the gain they'll get from adding kernel
code to get DOITM=1.  There are a range of ways we could handle it, all
the way from adding a command-line parameter to per-task management.

Anybody disagree?

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

[Peter Zijlstra]

On Tue, Aug 30, 2022 at 07:25:29AM -0700, Dave Hansen wrote:
> On 8/29/22 09:39, Jason A. Donenfeld wrote:
> > On Thu, Aug 25, 2022 at 11:15:58PM +0000, Eric Biggers wrote:
> >> I'm wondering if people are aware of this issue, and whether anyone has any
> >> thoughts on whether/where the kernel should be setting these new CPU flags.
> >> There don't appear to have been any prior discussions about this.  (Thanks to
> > Maybe it should be set unconditionally now, until we figure out how to
> > make it more granular.
> 
> Personally, I'm in this camp as well.  Let's be safe and set it by
> default.  There's also this tidbit in the Intel docs (and chopping out a
> bunch of the noise):
> 
> 	(On) processors based on microarchitectures before Ice Lake ...
> 	the instructions listed here operate as if DOITM is enabled.
> 
> IOW, setting DOITM=0 isn't going back to the stone age.  At worst, I'd
> guess that you're giving up some optimization that only shows up in very
> recent CPUs in the first place.
> 
> If folks want DOITM=1 on their snazzy new CPUs, then they came come with
> performance data to demonstrate the gain they'll get from adding kernel
> code to get DOITM=1.  There are a range of ways we could handle it, all
> the way from adding a command-line parameter to per-task management.
> 
> Anybody disagree?

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(-)

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 */
 
 /* Intel-defined CPU features, CPUID level 0x00000007:1 (EAX), word 12 */
 #define X86_FEATURE_AVX_VNNI		(12*32+ 4) /* AVX VNNI instructions */
@@ -460,5 +461,7 @@
 #define X86_BUG_MMIO_STALE_DATA		X86_BUG(25) /* CPU is affected by Processor MMIO Stale Data vulnerabilities */
 #define X86_BUG_RETBLEED		X86_BUG(26) /* CPU is affected by RETBleed */
 #define X86_BUG_EIBRS_PBRSB		X86_BUG(27) /* EIBRS is vulnerable to Post Barrier RSB Predictions */
+#define X86_BUG_DOIT			X86_BUG(28)
+#define X86_BUG_MCDT			X86_BUG(29)
 
 #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 */
+
 #define MSR_IA32_FLUSH_CMD		0x0000010b
 #define L1D_FLUSH			BIT(0)	/*
 						 * Writeback and invalidate the
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 28eba74b93de..e9d5bc870696 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -568,6 +568,30 @@ static __init int setup_disable_pku(char *arg)
 __setup("nopku", setup_disable_pku);
 #endif /* CONFIG_X86_64 */
 
+static bool doit_disabled = false;
+
+static __init int setup_disable_doit(char *arg)
+{
+	pr_info("x86: 'nodoit' specified, not enabling Data Operand Independent Timing\n");
+	doit_disabled = true;
+	return 1;
+}
+__setup("nodoit", setup_disable_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;
+
+	rdmsrl(MSR_IA32_UARCH_MISC_CTL, msr);
+	wrmsrl(MSR_IA32_UARCH_MISC_CTL, msr | UARCH_MISC_DOIT);
+}
+
 #ifdef CONFIG_X86_KERNEL_IBT
 
 __noendbr u64 ibt_save(void)
@@ -1249,6 +1273,8 @@ static const __initconst struct x86_cpu_id cpu_vuln_whitelist[] = {
 #define MMIO_SBDS	BIT(2)
 /* CPU is affected by RETbleed, speculating where you would not expect it */
 #define RETBLEED	BIT(3)
+/* CPU might be affected by MXCSR Configuration Dependent Timing (MCDT) */
+#define MCDT		BIT(4)
 
 static const struct x86_cpu_id cpu_vuln_blacklist[] __initconst = {
 	VULNBL_INTEL_STEPPINGS(IVYBRIDGE,	X86_STEPPING_ANY,		SRBDS),
@@ -1260,20 +1286,26 @@ static const struct x86_cpu_id cpu_vuln_blacklist[] __initconst = {
 	VULNBL_INTEL_STEPPINGS(BROADWELL_G,	X86_STEPPING_ANY,		SRBDS),
 	VULNBL_INTEL_STEPPINGS(BROADWELL_X,	X86_STEPPING_ANY,		MMIO),
 	VULNBL_INTEL_STEPPINGS(BROADWELL,	X86_STEPPING_ANY,		SRBDS),
-	VULNBL_INTEL_STEPPINGS(SKYLAKE_L,	X86_STEPPING_ANY,		SRBDS | MMIO | RETBLEED),
-	VULNBL_INTEL_STEPPINGS(SKYLAKE_X,	X86_STEPPING_ANY,		MMIO | RETBLEED),
-	VULNBL_INTEL_STEPPINGS(SKYLAKE,		X86_STEPPING_ANY,		SRBDS | MMIO | RETBLEED),
-	VULNBL_INTEL_STEPPINGS(KABYLAKE_L,	X86_STEPPING_ANY,		SRBDS | MMIO | RETBLEED),
-	VULNBL_INTEL_STEPPINGS(KABYLAKE,	X86_STEPPING_ANY,		SRBDS | MMIO | RETBLEED),
-	VULNBL_INTEL_STEPPINGS(CANNONLAKE_L,	X86_STEPPING_ANY,		RETBLEED),
-	VULNBL_INTEL_STEPPINGS(ICELAKE_L,	X86_STEPPING_ANY,		MMIO | MMIO_SBDS | RETBLEED),
-	VULNBL_INTEL_STEPPINGS(ICELAKE_D,	X86_STEPPING_ANY,		MMIO),
-	VULNBL_INTEL_STEPPINGS(ICELAKE_X,	X86_STEPPING_ANY,		MMIO),
-	VULNBL_INTEL_STEPPINGS(COMETLAKE,	X86_STEPPING_ANY,		MMIO | MMIO_SBDS | RETBLEED),
-	VULNBL_INTEL_STEPPINGS(COMETLAKE_L,	X86_STEPPINGS(0x0, 0x0),	MMIO | RETBLEED),
-	VULNBL_INTEL_STEPPINGS(COMETLAKE_L,	X86_STEPPING_ANY,		MMIO | MMIO_SBDS | RETBLEED),
-	VULNBL_INTEL_STEPPINGS(LAKEFIELD,	X86_STEPPING_ANY,		MMIO | MMIO_SBDS | RETBLEED),
-	VULNBL_INTEL_STEPPINGS(ROCKETLAKE,	X86_STEPPING_ANY,		MMIO | RETBLEED),
+	VULNBL_INTEL_STEPPINGS(SKYLAKE_L,	X86_STEPPING_ANY,		SRBDS | MMIO | RETBLEED | MCDT),
+	VULNBL_INTEL_STEPPINGS(SKYLAKE_X,	X86_STEPPING_ANY,		MMIO | RETBLEED | MCDT),
+	VULNBL_INTEL_STEPPINGS(SKYLAKE,		X86_STEPPING_ANY,		SRBDS | MMIO | RETBLEED | MCDT),
+	VULNBL_INTEL_STEPPINGS(KABYLAKE_L,	X86_STEPPING_ANY,		SRBDS | MMIO | RETBLEED | MCDT),
+	VULNBL_INTEL_STEPPINGS(KABYLAKE,	X86_STEPPING_ANY,		SRBDS | MMIO | RETBLEED | MCDT),
+	VULNBL_INTEL_STEPPINGS(CANNONLAKE_L,	X86_STEPPING_ANY,		RETBLEED | MCDT),
+	VULNBL_INTEL_STEPPINGS(ICELAKE_L,	X86_STEPPING_ANY,		MMIO | MMIO_SBDS | RETBLEED | MCDT),
+	VULNBL_INTEL_STEPPINGS(ICELAKE_D,	X86_STEPPING_ANY,		MMIO | MCDT),
+	VULNBL_INTEL_STEPPINGS(ICELAKE_X,	X86_STEPPING_ANY,		MMIO | MCDT),
+	VULNBL_INTEL_STEPPINGS(COMETLAKE,	X86_STEPPING_ANY,		MMIO | MMIO_SBDS | RETBLEED | MCDT),
+	VULNBL_INTEL_STEPPINGS(COMETLAKE_L,	X86_STEPPINGS(0x0, 0x0),	MMIO | RETBLEED | MCDT),
+	VULNBL_INTEL_STEPPINGS(COMETLAKE_L,	X86_STEPPING_ANY,		MMIO | MMIO_SBDS | RETBLEED | MCDT),
+	VULNBL_INTEL_STEPPINGS(LAKEFIELD,	X86_STEPPING_ANY,		MMIO | MMIO_SBDS | RETBLEED | MCDT),
+	VULNBL_INTEL_STEPPINGS(ROCKETLAKE,	X86_STEPPING_ANY,		MMIO | RETBLEED | MCDT),
+	VULNBL_INTEL_STEPPINGS(TIGERLAKE,	X86_STEPPING_ANY,		MCDT),
+	VULNBL_INTEL_STEPPINGS(TIGERLAKE_L,	X86_STEPPING_ANY,		MCDT),
+	VULNBL_INTEL_STEPPINGS(ALDERLAKE,	X86_STEPPING_ANY,		MCDT),
+	VULNBL_INTEL_STEPPINGS(ALDERLAKE_L,	X86_STEPPING_ANY,		MCDT),
+	VULNBL_INTEL_STEPPINGS(ALDERLAKE_N,	X86_STEPPING_ANY,		MCDT),
+
 	VULNBL_INTEL_STEPPINGS(ATOM_TREMONT,	X86_STEPPING_ANY,		MMIO | MMIO_SBDS),
 	VULNBL_INTEL_STEPPINGS(ATOM_TREMONT_D,	X86_STEPPING_ANY,		MMIO),
 	VULNBL_INTEL_STEPPINGS(ATOM_TREMONT_L,	X86_STEPPING_ANY,		MMIO | MMIO_SBDS),
@@ -1318,6 +1350,9 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
 	    !(ia32_cap & ARCH_CAP_PSCHANGE_MC_NO))
 		setup_force_cpu_bug(X86_BUG_ITLB_MULTIHIT);
 
+	if (ia32_cap & ARCH_CAP_DOIT)
+		setup_force_cpu_bug(X86_BUG_DOIT);
+
 	if (cpu_matches(cpu_vuln_whitelist, NO_SPECULATION))
 		return;
 
@@ -1388,6 +1423,11 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
 			setup_force_cpu_bug(X86_BUG_RETBLEED);
 	}
 
+	if (!cpu_has(c, X86_FEATURE_MCDT_NO)) {
+		if (cpu_matches(cpu_vuln_blacklist, MCDT))
+			setup_force_cpu_bug(X86_BUG_MCDT);
+	}
+
 	if (cpu_has(c, X86_FEATURE_IBRS_ENHANCED) &&
 	    !cpu_matches(cpu_vuln_whitelist, NO_EIBRS_PBRSB) &&
 	    !(ia32_cap & ARCH_CAP_PBRSB_NO))
@@ -1869,6 +1909,7 @@ static void identify_cpu(struct cpuinfo_x86 *c)
 	x86_init_rdrand(c);
 	setup_pku(c);
 	setup_cet(c);
+	setup_doit(c);
 
 	/*
 	 * Clear/Set all flags overridden by options, need do it
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 },
 	{ X86_FEATURE_RRSBA_CTRL,	CPUID_EDX,  2, 0x00000007, 2 },
 	{ X86_FEATURE_CQM_LLC,		CPUID_EDX,  1, 0x0000000f, 0 },
 	{ X86_FEATURE_CQM_OCCUP_LLC,	CPUID_EDX,  0, 0x0000000f, 1 },

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

[Eric Biggers]

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

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

[Catalin Marinas]

On Tue, Aug 30, 2022 at 07:25:29AM -0700, Dave Hansen wrote:
> On 8/29/22 09:39, Jason A. Donenfeld wrote:
> > On Thu, Aug 25, 2022 at 11:15:58PM +0000, Eric Biggers wrote:
> >> I'm wondering if people are aware of this issue, and whether anyone has any
> >> thoughts on whether/where the kernel should be setting these new CPU flags.
> >> There don't appear to have been any prior discussions about this.  (Thanks to
> > Maybe it should be set unconditionally now, until we figure out how to
> > make it more granular.
> 
> Personally, I'm in this camp as well.  Let's be safe and set it by
> default.  There's also this tidbit in the Intel docs (and chopping out a
> bunch of the noise):
> 
> 	(On) processors based on microarchitectures before Ice Lake ...
> 	the instructions listed here operate as if DOITM is enabled.
> 
> IOW, setting DOITM=0 isn't going back to the stone age.  At worst, I'd
> guess that you're giving up some optimization that only shows up in very
> recent CPUs in the first place.
> 
> If folks want DOITM=1 on their snazzy new CPUs, then they came come with
> performance data to demonstrate the gain they'll get from adding kernel
> code to get DOITM=1.  There are a range of ways we could handle it, all
> the way from adding a command-line parameter to per-task management.
> 
> Anybody disagree?

It's not my preferred option for arm64 but I admit the same reasoning
could equally apply to us. If some existing crypto libraries relied on
data independent timing for current CPUs but newer ones (with the DIT
feature) come up with more aggressive, data-dependent optimisations,
they may be caught off-guard. That said the ARM architecture spec never
promised any timing, that's a micro-architecture detail and not all
implementations are done by ARM Ltd. So I can't really tell what's out
there.

So I guess knobs for finer grained control would do, at least a sysctl
(or cmdline) to turn it on/off globally and maybe a prctl() for user. We
don't necessarily need this on arm64 but if x86 adds one, we might as
well wire it up.

-- 
Catalin

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

[Ard Biesheuvel]

On Thu, 15 Sept 2022 at 19:52, Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Tue, Aug 30, 2022 at 07:25:29AM -0700, Dave Hansen wrote:
> > On 8/29/22 09:39, Jason A. Donenfeld wrote:
> > > On Thu, Aug 25, 2022 at 11:15:58PM +0000, Eric Biggers wrote:
> > >> I'm wondering if people are aware of this issue, and whether anyone has any
> > >> thoughts on whether/where the kernel should be setting these new CPU flags.
> > >> There don't appear to have been any prior discussions about this.  (Thanks to
> > > Maybe it should be set unconditionally now, until we figure out how to
> > > make it more granular.
> >
> > Personally, I'm in this camp as well.  Let's be safe and set it by
> > default.  There's also this tidbit in the Intel docs (and chopping out a
> > bunch of the noise):
> >
> >       (On) processors based on microarchitectures before Ice Lake ...
> >       the instructions listed here operate as if DOITM is enabled.
> >
> > IOW, setting DOITM=0 isn't going back to the stone age.  At worst, I'd
> > guess that you're giving up some optimization that only shows up in very
> > recent CPUs in the first place.
> >
> > If folks want DOITM=1 on their snazzy new CPUs, then they came come with
> > performance data to demonstrate the gain they'll get from adding kernel
> > code to get DOITM=1.  There are a range of ways we could handle it, all
> > the way from adding a command-line parameter to per-task management.
> >
> > Anybody disagree?
>
> It's not my preferred option for arm64 but I admit the same reasoning
> could equally apply to us. If some existing crypto libraries relied on
> data independent timing for current CPUs but newer ones (with the DIT
> feature) come up with more aggressive, data-dependent optimisations,
> they may be caught off-guard. That said the ARM architecture spec never
> promised any timing, that's a micro-architecture detail and not all
> implementations are done by ARM Ltd. So I can't really tell what's out
> there.
>
> So I guess knobs for finer grained control would do, at least a sysctl
> (or cmdline) to turn it on/off globally and maybe a prctl() for user. We
> don't necessarily need this on arm64 but if x86 adds one, we might as
> well wire it up.
>

With all the effort spent on plugging timing leaks in the kernel over
the past couple of years, not enabling this at EL1 seems silly, no?
Why would we ever permit privileged code to exhibit data dependent
timing variances?

As for a prctl() for user space - wouldn't it make more sense to
enable this by default, and add a hwcap so user space can clear DIT
directly if it feels the need to do so?