Messages in this thread Patch in this message | | | Date | Tue, 8 Nov 2022 14:41:26 +0000 | From | Mark Rutland <> | Subject | Re: [PATCH v2] arm64: Enable data independent timing (DIT) in the kernel |
| |
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 >
| |