diff mbox series

[v2] arm64: Enable data independent timing (DIT) in the kernel

Message ID 20221107172400.1851434-1-ardb@kernel.org
State New
Headers
Series [v2] arm64: Enable data independent timing (DIT) in the kernel |

Commit Message

Ard Biesheuvel Nov. 7, 2022, 5:24 p.m. UTC 
  The ARM architecture revision v8.4 introduces a data independent timing
control (DIT) which can be set at any exception level, and instructs the
CPU to avoid optimizations that may result in a correlation between the
execution time of certain instructions and the value of the data they
operate on.

The DIT bit is part of PSTATE, and is therefore context switched as
usual, given that it becomes part of the saved program state (SPSR) when
taking an exception. We have also defined a hwcap for DIT, and so user
space can discover already whether or nor DIT is available. This means
that, as far as user space is concerned, DIT is wired up and fully
functional.

In the kernel, however, we never bothered with DIT: we disable at it
boot (i.e., INIT_PSTATE_EL1 has DIT cleared) and ignore the fact that we
might run with DIT enabled if user space happened to set it.

Currently, we have no idea whether or not running privileged code with
DIT disabled on a CPU that implements support for it may result in a
side channel that exposes privileged data to unprivileged user space
processes, so let's be cautious and just enable DIT while running in the
kernel if supported by all CPUs.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Eric Biggers <ebiggers@kernel.org>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Adam Langley <agl@google.com>
Link: https://lore.kernel.org/all/YwgCrqutxmX0W72r@gmail.com/
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
v2:
- enable DIT on resume-from-suspend path
- tidy up some issues spotted by Eric
- avoid some code duplication in SET_PSTATE_xxx macro definitions
- tweak the commit log so that it doesn't read as if we are fixing an
  actual vulnerability

 arch/arm64/include/asm/cpufeature.h |  5 +++++
 arch/arm64/include/asm/sysreg.h     | 12 ++++++++----
 arch/arm64/kernel/cpufeature.c      | 17 +++++++++++++++++
 arch/arm64/kernel/entry.S           |  3 +++
 arch/arm64/kernel/suspend.c         |  2 ++
 arch/arm64/tools/cpucaps            |  1 +
 6 files changed, 36 insertions(+), 4 deletions(-)

Comments

Eric Biggers Nov. 7, 2022, 8:38 p.m. UTC | #1 
On Mon, Nov 07, 2022 at 06:24:00PM +0100, Ard Biesheuvel wrote:
[...]
> 
> Currently, we have no idea whether or not running privileged code with
> DIT disabled on a CPU that implements support for it may result in a
> side channel that exposes privileged data to unprivileged user space
> processes, so let's be cautious and just enable DIT while running in the
> kernel if supported by all CPUs.
[...]
> 
> - tweak the commit log so that it doesn't read as if we are fixing an
>   actual vulnerability

I think the above undersells this a bit, as crypto code often relies on
instructions being constant-time to prevent leakage of secrets outside the
system itself.  For example, consider WireGuard, which includes network
attackers in its threat model.  So it's not just about attacks from userspace
processes on the same system.

The patch itself looks good to me though -- thanks!

- Eric
Mark Rutland Nov. 8, 2022, 2:41 p.m. UTC | #2 
Hi Ard,

On Mon, Nov 07, 2022 at 06:24:00PM +0100, Ard Biesheuvel wrote:
> The ARM architecture revision v8.4 introduces a data independent timing
> control (DIT) which can be set at any exception level, and instructs the
> CPU to avoid optimizations that may result in a correlation between the
> execution time of certain instructions and the value of the data they
> operate on.
> 
> The DIT bit is part of PSTATE, and is therefore context switched as
> usual, given that it becomes part of the saved program state (SPSR) when
> taking an exception. We have also defined a hwcap for DIT, and so user
> space can discover already whether or nor DIT is available. This means
> that, as far as user space is concerned, DIT is wired up and fully
> functional.
> 
> In the kernel, however, we never bothered with DIT: we disable at it
> boot (i.e., INIT_PSTATE_EL1 has DIT cleared) and ignore the fact that we
> might run with DIT enabled if user space happened to set it.
> 
> Currently, we have no idea whether or not running privileged code with
> DIT disabled on a CPU that implements support for it may result in a
> side channel that exposes privileged data to unprivileged user space
> processes, so let's be cautious and just enable DIT while running in the
> kernel if supported by all CPUs.

Thanks for respinning the wording!

I have one minor nit below, but otherwise this looks good to me.

> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index f73f11b5504254be..f44579bca9f8107e 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -875,6 +875,11 @@ static inline bool cpu_has_pan(void)
>  						    ID_AA64MMFR1_EL1_PAN_SHIFT);
>  }
>  
> +static inline bool cpu_has_dit(void)
> +{
> +	return cpus_have_const_cap(ARM64_HAS_DIT);
> +}

Normally cpu_has_X() implies a local feature check, and cpus_have_X() tests for
common support, so this should be cpus_have_dit().

That said, this is only used in one place below, so we could use the CAP
directly there without a wrapper.

> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 7d301700d1a93692..1f3f52ce407fe942 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -90,20 +90,24 @@
>   */
>  #define pstate_field(op1, op2)		((op1) << Op1_shift | (op2) << Op2_shift)
>  #define PSTATE_Imm_shift		CRm_shift
> +#define SET_PSTATE(x, r)		__emit_inst(0xd500401f | PSTATE_ ## r | ((!!x) << PSTATE_Imm_shift))
>  
>  #define PSTATE_PAN			pstate_field(0, 4)
>  #define PSTATE_UAO			pstate_field(0, 3)
>  #define PSTATE_SSBS			pstate_field(3, 1)
> +#define PSTATE_DIT			pstate_field(3, 2)
>  #define PSTATE_TCO			pstate_field(3, 4)
>  
> -#define SET_PSTATE_PAN(x)		__emit_inst(0xd500401f | PSTATE_PAN | ((!!x) << PSTATE_Imm_shift))
> -#define SET_PSTATE_UAO(x)		__emit_inst(0xd500401f | PSTATE_UAO | ((!!x) << PSTATE_Imm_shift))
> -#define SET_PSTATE_SSBS(x)		__emit_inst(0xd500401f | PSTATE_SSBS | ((!!x) << PSTATE_Imm_shift))
> -#define SET_PSTATE_TCO(x)		__emit_inst(0xd500401f | PSTATE_TCO | ((!!x) << PSTATE_Imm_shift))
> +#define SET_PSTATE_PAN(x)		SET_PSTATE((x), PAN)
> +#define SET_PSTATE_UAO(x)		SET_PSTATE((x), UAO)
> +#define SET_PSTATE_SSBS(x)		SET_PSTATE((x), SSBS)
> +#define SET_PSTATE_DIT(x)		SET_PSTATE((x), DIT)
> +#define SET_PSTATE_TCO(x)		SET_PSTATE((x), TCO)

Nice!

[...]

> diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c
> index 8b02d310838f9240..3032a82ea51a19f7 100644
> --- a/arch/arm64/kernel/suspend.c
> +++ b/arch/arm64/kernel/suspend.c
> @@ -60,6 +60,8 @@ void notrace __cpu_suspend_exit(void)
>  	 * PSTATE was not saved over suspend/resume, re-enable any detected
>  	 * features that might not have been set correctly.
>  	 */
> +	if (cpu_has_dit())
> +		set_pstate_dit(1);

As above, I'd prefer if we either renamed cpu_has_dit() to cpus_have_dit(), or
just open-coded this as:

	if (cpus_have_const_cap(ARM64_HAS_DIT))
		set_pstate_dit(1);

With either of those options:

  Acked-by: Mark Rutland <mark.rutland@arm.com>

I assume Will might fix that up when applying.

Mark.

>  	__uaccess_enable_hw_pan();
>  
>  	/*
> diff --git a/arch/arm64/tools/cpucaps b/arch/arm64/tools/cpucaps
> index f1c0347ec31a85c7..a86ee376920a08dd 100644
> --- a/arch/arm64/tools/cpucaps
> +++ b/arch/arm64/tools/cpucaps
> @@ -20,6 +20,7 @@ HAS_CNP
>  HAS_CRC32
>  HAS_DCPODP
>  HAS_DCPOP
> +HAS_DIT
>  HAS_E0PD
>  HAS_ECV
>  HAS_EPAN
> -- 
> 2.35.1
>
Will Deacon Nov. 8, 2022, 2:56 p.m. UTC | #3 
On Tue, Nov 08, 2022 at 02:41:26PM +0000, Mark Rutland wrote:
> On Mon, Nov 07, 2022 at 06:24:00PM +0100, Ard Biesheuvel wrote:
> > diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c
> > index 8b02d310838f9240..3032a82ea51a19f7 100644
> > --- a/arch/arm64/kernel/suspend.c
> > +++ b/arch/arm64/kernel/suspend.c
> > @@ -60,6 +60,8 @@ void notrace __cpu_suspend_exit(void)
> >  	 * PSTATE was not saved over suspend/resume, re-enable any detected
> >  	 * features that might not have been set correctly.
> >  	 */
> > +	if (cpu_has_dit())
> > +		set_pstate_dit(1);
> 
> As above, I'd prefer if we either renamed cpu_has_dit() to cpus_have_dit(), or
> just open-coded this as:
> 
> 	if (cpus_have_const_cap(ARM64_HAS_DIT))
> 		set_pstate_dit(1);
> 
> With either of those options:
> 
>   Acked-by: Mark Rutland <mark.rutland@arm.com>
> 
> I assume Will might fix that up when applying.

Yup, I'll take care of that.

Will
Will Deacon Nov. 8, 2022, 5:38 p.m. UTC | #4 
On Mon, 7 Nov 2022 18:24:00 +0100, Ard Biesheuvel wrote:
> The ARM architecture revision v8.4 introduces a data independent timing
> control (DIT) which can be set at any exception level, and instructs the
> CPU to avoid optimizations that may result in a correlation between the
> execution time of certain instructions and the value of the data they
> operate on.
> 
> The DIT bit is part of PSTATE, and is therefore context switched as
> usual, given that it becomes part of the saved program state (SPSR) when
> taking an exception. We have also defined a hwcap for DIT, and so user
> space can discover already whether or nor DIT is available. This means
> that, as far as user space is concerned, DIT is wired up and fully
> functional.
> 
> [...]

Applied to arm64 (for-next/cpufeature), thanks!

[1/1] arm64: Enable data independent timing (DIT) in the kernel
      https://git.kernel.org/arm64/c/01ab991fc0ee 

Cheers,
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index f73f11b5504254be..f44579bca9f8107e 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -875,6 +875,11 @@  static inline bool cpu_has_pan(void)
 						    ID_AA64MMFR1_EL1_PAN_SHIFT);
 }
 
+static inline bool cpu_has_dit(void)
+{
+	return cpus_have_const_cap(ARM64_HAS_DIT);
+}
+
 #ifdef CONFIG_ARM64_AMU_EXTN
 /* Check whether the cpu supports the Activity Monitors Unit (AMU) */
 extern bool cpu_has_amu_feat(int cpu);
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 7d301700d1a93692..1f3f52ce407fe942 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -90,20 +90,24 @@ 
  */
 #define pstate_field(op1, op2)		((op1) << Op1_shift | (op2) << Op2_shift)
 #define PSTATE_Imm_shift		CRm_shift
+#define SET_PSTATE(x, r)		__emit_inst(0xd500401f | PSTATE_ ## r | ((!!x) << PSTATE_Imm_shift))
 
 #define PSTATE_PAN			pstate_field(0, 4)
 #define PSTATE_UAO			pstate_field(0, 3)
 #define PSTATE_SSBS			pstate_field(3, 1)
+#define PSTATE_DIT			pstate_field(3, 2)
 #define PSTATE_TCO			pstate_field(3, 4)
 
-#define SET_PSTATE_PAN(x)		__emit_inst(0xd500401f | PSTATE_PAN | ((!!x) << PSTATE_Imm_shift))
-#define SET_PSTATE_UAO(x)		__emit_inst(0xd500401f | PSTATE_UAO | ((!!x) << PSTATE_Imm_shift))
-#define SET_PSTATE_SSBS(x)		__emit_inst(0xd500401f | PSTATE_SSBS | ((!!x) << PSTATE_Imm_shift))
-#define SET_PSTATE_TCO(x)		__emit_inst(0xd500401f | PSTATE_TCO | ((!!x) << PSTATE_Imm_shift))
+#define SET_PSTATE_PAN(x)		SET_PSTATE((x), PAN)
+#define SET_PSTATE_UAO(x)		SET_PSTATE((x), UAO)
+#define SET_PSTATE_SSBS(x)		SET_PSTATE((x), SSBS)
+#define SET_PSTATE_DIT(x)		SET_PSTATE((x), DIT)
+#define SET_PSTATE_TCO(x)		SET_PSTATE((x), TCO)
 
 #define set_pstate_pan(x)		asm volatile(SET_PSTATE_PAN(x))
 #define set_pstate_uao(x)		asm volatile(SET_PSTATE_UAO(x))
 #define set_pstate_ssbs(x)		asm volatile(SET_PSTATE_SSBS(x))
+#define set_pstate_dit(x)		asm volatile(SET_PSTATE_DIT(x))
 
 #define __SYS_BARRIER_INSN(CRm, op2, Rt) \
 	__emit_inst(0xd5000000 | sys_insn(0, 3, 3, (CRm), (op2)) | ((Rt) & 0x1f))
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 6062454a90674317..74ceec411ea8b597 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -2077,6 +2077,11 @@  static void cpu_trap_el0_impdef(const struct arm64_cpu_capabilities *__unused)
 	sysreg_clear_set(sctlr_el1, 0, SCTLR_EL1_TIDCP);
 }
 
+static void cpu_enable_dit(const struct arm64_cpu_capabilities *__unused)
+{
+	set_pstate_dit(1);
+}
+
 /* Internal helper functions to match cpu capability type */
 static bool
 cpucap_late_cpu_optional(const struct arm64_cpu_capabilities *cap)
@@ -2640,6 +2645,18 @@  static const struct arm64_cpu_capabilities arm64_features[] = {
 		.matches = has_cpuid_feature,
 		.cpu_enable = cpu_trap_el0_impdef,
 	},
+	{
+		.desc = "Data independent timing control (DIT)",
+		.capability = ARM64_HAS_DIT,
+		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
+		.sys_reg = SYS_ID_AA64PFR0_EL1,
+		.sign = FTR_UNSIGNED,
+		.field_pos = ID_AA64PFR0_EL1_DIT_SHIFT,
+		.field_width = 4,
+		.min_field_value = ID_AA64PFR0_EL1_DIT_IMP,
+		.matches = has_cpuid_feature,
+		.cpu_enable = cpu_enable_dit,
+	},
 	{},
 };
 
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index e28137d64b7688e2..11cb99c4d298784d 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -197,6 +197,9 @@  alternative_cb_end
 	.endm
 
 	.macro	kernel_entry, el, regsize = 64
+	.if	\el == 0
+	alternative_insn nop, SET_PSTATE_DIT(1), ARM64_HAS_DIT
+	.endif
 	.if	\regsize == 32
 	mov	w0, w0				// zero upper 32 bits of x0
 	.endif
diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c
index 8b02d310838f9240..3032a82ea51a19f7 100644
--- a/arch/arm64/kernel/suspend.c
+++ b/arch/arm64/kernel/suspend.c
@@ -60,6 +60,8 @@  void notrace __cpu_suspend_exit(void)
 	 * PSTATE was not saved over suspend/resume, re-enable any detected
 	 * features that might not have been set correctly.
 	 */
+	if (cpu_has_dit())
+		set_pstate_dit(1);
 	__uaccess_enable_hw_pan();
 
 	/*
diff --git a/arch/arm64/tools/cpucaps b/arch/arm64/tools/cpucaps
index f1c0347ec31a85c7..a86ee376920a08dd 100644
--- a/arch/arm64/tools/cpucaps
+++ b/arch/arm64/tools/cpucaps
@@ -20,6 +20,7 @@  HAS_CNP
 HAS_CRC32
 HAS_DCPODP
 HAS_DCPOP
+HAS_DIT
 HAS_E0PD
 HAS_ECV
 HAS_EPAN