arm64: Enable data independent timing (DIT) in the kernel
RFC PATCH 1/1 arm64: Enable data independent timing (DIT) in the kernel
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.
Given that 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, let's enable DIT while running in the kernel if supported by all CPUs.
--- arch/arm64/include/asm/sysreg.h | 3 +++ arch/arm64/kernel/cpufeature.c | 16 ++++++++++++++++ arch/arm64/kernel/entry.S | 3 +++ arch/arm64/tools/cpucaps | 1 + 4 files changed, 23 insertions(+)
arch/arm64/include/asm/sysreg.h +3 -0
@@ -94,15 +94,18 @@#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_DIT (x) __emit_inst(0xd500401f | PSTATE_DIT | ((!!x) << PSTATE_Imm_shift))#define SET_PSTATE_TCO (x) __emit_inst(0xd500401f | PSTATE_TCO | ((!!x) << PSTATE_Imm_shift))#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_dit (x) asm volatile(SET_PSTATE_DIT(x))#define set_pstate_ssbs (x) asm volatile(SET_PSTATE_SSBS(x))#define __SYS_BARRIER_INSN (CRm, op2, Rt) \
arch/arm64/kernel/cpufeature.c +16 -0
@@ -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,17 @@ 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, + .matches = has_cpuid_feature, + .sys_reg = SYS_ID_AA64PFR0_EL1, + .field_pos = ID_AA64PFR0_EL1_DIT_SHIFT, + .field_width = 4, + .min_field_value = 1, + .cpu_enable = cpu_enable_dit, + }, {}, };
arch/arm64/kernel/entry.S +3 -0
@@ -197,6 +197,9 @@ alternative_cb_end .endm .macro kernel_entry, el, regsize = 64 + .if \el == 0 + ALTERNATIVE(nop, SET_PSTATE_DIT(1), ARM64_HAS_DIT) + .endif .if \regsize == 32 mov w0, w0 // zero upper 32 bits of x0 .endif
arch/arm64/tools/cpucaps +1 -0
@@ -20,6 +20,7 @@ HAS_CNP HAS_CRC32 HAS_DCPODP HAS_DCPOP +HAS_DIT HAS_E0PD HAS_ECV HAS_EPAN
Signature · 1 line
2.35.1
Mark Rutland
reviewerQuoted 13 lines · Ard Biesheuvel
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.
Quoted 3 lines
In the kernel, however, we never bothered with DIT: we disable at it boot (i.e.,
INIT_PSTATE_EL1has DIT cleared) and ignore the fact that we might run with DIT enabled if user space happened to set it.
FWIW, I did raise this with some architects a while back, and the thinking at the time was that implementations were likely to have data independent timing regardless of the value of the DIT bit, and so manipulating this at exception entry was busy work with no actual gain (especially if we had to handle mismatched big.LITTLE systems).
Since then, I have become aware of some possible implementation choices which would consider the bit (though I have no idea if anyone is building such implementations).
Quoted 3 lines
Given that 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,
I appreciate this is a simple way to rule out issues of that sort, but I think the "may" in that sentence is doing a lot of work, since:
* IIUC, we don't have a specific case in mind that we're concerned about. I can believe that we think all the crypto code we intend to be constant time is theoretically affected.
* IIUC we haven't gone an audited all constant-time code to check it doesn't happen to use instructions with data-dependent-timing. So there might be more work to do atop this to ensure theoretical correctness.
* AFAIK there are no contemporary implementations where the DIT is both implemented and it being clear results in data-dependent-timing. i.e. we have nothing to actually test on.
Given that, it would be nice if we could make this a bit clearer, e.g. state that this is currently a belt-and-braces approach for theoretical cases, rather than an extant side-channel today (as far as we're aware).
Quoted 1 line
let's enable DIT while running in the kernel if supported by all CPUs.
FWIW, I think it's reasonable to do this when all CPUs have DIT.
I have a slight fear that (as above) if there are future CPUs which do consider DIT, there's presumably a noticeable performance difference (or the CPU would just provide data-independent-timing regardless), but I'm not sure if that's just something we have to live with or could punt on until we notice such cases.
Quoted 11 lines
Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Mark Rutland <[email protected]>Cc Marc Zyngier <[email protected]>Cc Eric Biggers <[email protected]>Cc Jason A. Donenfeld <[email protected]>Cc Kees Cook <[email protected]>Cc Suzuki K Poulose <[email protected]>Cc Adam Langley <[email protected]>Signed-off-by Ard Biesheuvel <[email protected]>
Quoted 6 lines
--- arch/arm64/include/asm/sysreg.h | 3 +++ arch/arm64/kernel/cpufeature.c | 16 ++++++++++++++++ arch/arm64/kernel/entry.S | 3 +++ arch/arm64/tools/cpucaps | 1 + 4 files changed, 23 insertions(+)
Don't we also need to touch __cpu_suspend_exit() in arch/am64/kernel/suspend.c? I'm assuming so given that has __uaccess_enable_hw_pan() today.
Presumably we'd also care about this in KVM hyp code?
Quoted 28 lines
arch/arm64/include/asm/sysreg.h +3
-0@@ -94,15 +94,18 @@#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_DIT (x) __emit_inst(0xd500401f | PSTATE_DIT | ((!!x) << PSTATE_Imm_shift))#define SET_PSTATE_TCO (x) __emit_inst(0xd500401f | PSTATE_TCO | ((!!x) << PSTATE_Imm_shift))#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_dit (x) asm volatile(SET_PSTATE_DIT(x))#define set_pstate_ssbs (x) asm volatile(SET_PSTATE_SSBS(x))#define __SYS_BARRIER_INSN (CRm, op2, Rt) \arch/arm64/kernel/cpufeature.c +0
-0
arch/arm64/kernel/cpufeature.c static void cpu_trap_el0_impdef(const struct arm64_cpu_capabilities *__unused) · line 2083
Quoted 7 lines
arch/arm64/kernel/cpufeature.c +4
-0@@ -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 ); +}
I think this wants the same treatment as PAN, i.e. WARN_ON_ONCE(in_interrupt()); with a comment about PSTATE being discareded upon ERET.
Thanks,
Mark.
Quoted 51 lines
arch/arm64/kernel/cpufeature.c +12
-0+/* Internal helper functions to match cpu capability type */ static bool cpucap_late_cpu_optional (const struct arm64_cpu_capabilities *cap)@@ -2640,6 +2645,17 @@ 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, + .matches = has_cpuid_feature, + .sys_reg = SYS_ID_AA64PFR0_EL1, + .field_pos = ID_AA64PFR0_EL1_DIT_SHIFT, + .field_width = 4, + .min_field_value = 1, + .cpu_enable = cpu_enable_dit, + }, {}, };arch/arm64/kernel/entry.S +3
-0@@ -197,6 +197,9 @@ alternative_cb_end .endm .macro kernel_entry, el, regsize = 64 + .if \el == 0 + ALTERNATIVE(nop, SET_PSTATE_DIT(1), ARM64_HAS_DIT) + .endif .if \regsize == 32 mov w0, w0 // zero upper 32 bits of x0 .endifarch/arm64/tools/cpucaps +1
-0@@ -20,6 +20,7 @@ HAS_CNP HAS_CRC32 HAS_DCPODP HAS_DCPOP +HAS_DIT HAS_E0PD HAS_ECV HAS_EPANSignature · 1 line
2.35.1
Quoted 15 lines · Mark Rutland
I appreciate this is a simple way to rule out issues of that sort, but I think the "may" in that sentence is doing a lot of work, since:
* IIUC, we don't have a specific case in mind that we're concerned about. I can believe that we think all the crypto code we intend to be constant time is theoretically affected.
* IIUC we haven't gone an audited all constant-time code to check it doesn't happen to use instructions with data-dependent-timing. So there might be more work to do atop this to ensure theoretical correctness.
* AFAIK there are no contemporary implementations where the DIT is both implemented and it being clear results in data-dependent-timing. i.e. we have nothing to actually test on.
Quoted 5 lines
I have a slight fear that (as above) if there are future CPUs which do consider DIT, there's presumably a noticeable performance difference (or the CPU would just provide data-independent-timing regardless), but I'm not sure if that's just something we have to live with or could punt on until we notice such cases.
You're heading on a road to disaster reasoning like that.
You wrote, "we don't have a specific case in mind that we're concerned about", but actually, all you can say here is that you're not personally aware of a specific case in mind to be concerned about. As somebody who actually works on this code, I do have specific cases I'm worried about, and I know there are certain assumptions I've made about various coding patterns being CT, resulting in various instructions that I assume to be CT, which is something I tend to check by hand, while others have entire frameworks to automatically verify this kind of thing. In other words, one man's theory is another man's practice.
Then you write that there aren't any contemporary instructions where this matters, but you fear they could come up in the future. Okay, good, that's a perspective we can both share. The logical thing to do about that would be Ard's patch here. However, you then conclude something vague about performance and suggest punting this down the road. I guess this makes sense to you because you don't think timing attacks are real anyway. You're entitled to your opinion, of course, but I don't think it's a popular one, and it certainly is contrary to that of most implementers of the concerned code.
On the contrary, we should be proactive in ensuring the kernel remains a suitable environment for CT code, preventing the problem *before* it happens, rather than having to deal with vulnerability response down the road, "punt[ing]" it, as you said. And perhaps if we handle this now, CPU designers also won't feel like they can get away with silly performance gains at the cost of CT instructions.
Jason
Mark Rutland
reviewerHi Jason,
I think my wording below might have been unclear -- I was not arguing to punt on this patch. I've tried to clarify that below. Tone is always hard in text...
Quoted 24 lines · Jason A. Donenfeld
Quoted 15 lines · Mark Rutland
I appreciate this is a simple way to rule out issues of that sort, but I think the "may" in that sentence is doing a lot of work, since:
* IIUC, we don't have a specific case in mind that we're concerned about. I can believe that we think all the crypto code we intend to be constant time is theoretically affected.
* IIUC we haven't gone an audited all constant-time code to check it doesn't happen to use instructions with data-dependent-timing. So there might be more work to do atop this to ensure theoretical correctness.
* AFAIK there are no contemporary implementations where the DIT is both implemented and it being clear results in data-dependent-timing. i.e. we have nothing to actually test on.
Quoted 5 lines
I have a slight fear that (as above) if there are future CPUs which do consider DIT, there's presumably a noticeable performance difference (or the CPU would just provide data-independent-timing regardless), but I'm not sure if that's just something we have to live with or could punt on until we notice such cases.
You're heading on a road to disaster reasoning like that.
Quoted 9 lines
You wrote, "we don't have a specific case in mind that we're concerned about", but actually, all you can say here is that you're not personally aware of a specific case in mind to be concerned about. As somebody who actually works on this code, I do have specific cases I'm worried about, and I know there are certain assumptions I've made about various coding patterns being CT, resulting in various instructions that I assume to be CT, which is something I tend to check by hand, while others have entire frameworks to automatically verify this kind of thing. In other words, one man's theory is another man's practice.
What I'm trying to say here is that as-written the "may result in a side channel" wording is sufficiently vague that it can easily be read both more strongly and wore weakly than may have been intended (and can easily be editotorialised to "there's a new side-channel bug" style reporting that isn't helpful). Hence I'm calling that out explicitly for the benefit of others.
If we say something like "in the absence of DIT we can't ensure that instructions sequences have data independent timing, and so where this matters there could in theory be a side channel", or something like that, I'd be happier.
As you say, people have different levels of riguour here, and (unfortunately) there's plenty of extant code with informal assumptions that are hard to reason about. If you have a specific example to contribute to the commit messge, that would be great.
Quoted 5 lines
Then you write that there aren't any contemporary instructions where this matters, but you fear they could come up in the future. Okay, good, that's a perspective we can both share. The logical thing to do about that would be Ard's patch here. However, you then conclude something vague about performance and suggest punting this down the road.
Sorry, I think my wording was unclear here: I meant punting on the performance concern (i.e. take Ard's patch now, and if we see a performance issue in future, address it then). I think we're actually agreed on that?
My reason for noting the performance concern was to lampshade that for anyone aware of any extant / in-progress implementations, both to get them to tell their HW folk that they might not get the practical gains they expect, and since if those do exist we could consider following up with mechanisms to opt-out of data-independent-timing where we are both satisifed that doing so is safe and where there's a noticeable performance impact.
Quoted 4 lines
I guess this makes sense to you because you don't think timing attacks are real anyway. You're entitled to your opinion, of course, but I don't think it's a popular one, and it certainly is contrary to that of most implementers of the concerned code.
That is not at all what I was trying to say here, but I can see how it's possible to read that, so apologies for not being clear.
I am well aware that timing attacks are real, having specnt quite a while on spectre its acquaintances, and having argued for stronger architectural guarantees / better mechanisms to deal with this sort of thing.
Quoted 3 lines
On the contrary, we should be proactive in ensuring the kernel remains a suitable environment for CT code, preventing the problem *before* it happens,
I agree. I'm just trying to make sure that we're clear that we're doing that and this isn't a fix for an extant seurity bug which is actively exploitable today.
Quoted 4 lines
rather than having to deal with vulnerability response down the road, "punt[ing]" it, as you said. And perhaps if we handle this now, CPU designers also won't feel like they can get away with silly performance gains at the cost of CT instructions.
Sure.
Thanks,
Mark.
Quoted 25 lines · Mark Rutland
Quoted 13 lines · Ard Biesheuvel
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.
Quoted 3 lines
In the kernel, however, we never bothered with DIT: we disable at it boot (i.e.,
INIT_PSTATE_EL1has DIT cleared) and ignore the fact that we might run with DIT enabled if user space happened to set it.FWIW, I did raise this with some architects a while back, and the thinking at the time was that implementations were likely to have data independent timing regardless of the value of the DIT bit, and so manipulating this at exception entry was busy work with no actual gain (especially if we had to handle mismatched big.LITTLE systems).
Quoted 4 lines
Since then, I have become aware of some possible implementation choices which would consider the bit (though I have no idea if anyone is building such implementations).
It's a bit unfortunate that FEAT_DIT is mandatory even if the uarch in question behaves the same regardless. And the fact that it is documented as resetting to 0x0, and already being exposed to user space doesn't help either. But that doesn't justify keeping this disabled in the kernel.
The architecture does not permit us to distinguish between the cases where this is just busywork and where it does make a difference. So we have to assume it is the latter.
Quoted 11 lines
Quoted 3 lines
Given that 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,
I appreciate this is a simple way to rule out issues of that sort, but I think the "may" in that sentence is doing a lot of work, since:
* IIUC, we don't have a specific case in mind that we're concerned about. I can believe that we think all the crypto code we intend to be constant time is theoretically affected.
I think this reasoning is backwards. I don't think it is necessary to go and identify where we are relying on timing invariance. Crypto keeps coming up, and of course, it is a well known example of where timing variances may leak the plaintext or the key. But crypto is just one way to keep data confidential, and another method we rely on heavily is the privilege boundary between kernel and user space. So just like all crypto should be constant time, all privileged execution should [ideally] be constant time as well.
Quoted 4 lines
* IIUC we haven't gone an audited all constant-time code to check it doesn't happen to use instructions with data-dependent-timing. So there might be more work to do atop this to ensure theoretical correctness.
Sure.
Quoted 4 lines
* AFAIK there are no contemporary implementations where the DIT is both implemented and it being clear results in data-dependent-timing. i.e. we have nothing to actually test on.
Then why on earth are such implementations required to implement FEAT_DIT??
Quoted 4 lines
Given that, it would be nice if we could make this a bit clearer, e.g. state that this is currently a belt-and-braces approach for theoretical cases, rather than an extant side-channel today (as far as we're aware).
Sure - I'll add some extra prose to the commit log to capture the above.
Quoted 10 lines
Quoted 1 line
let's enable DIT while running in the kernel if supported by all CPUs.
FWIW, I think it's reasonable to do this when all CPUs have DIT.
I have a slight fear that (as above) if there are future CPUs which do consider DIT, there's presumably a noticeable performance difference (or the CPU would just provide data-independent-timing regardless), but I'm not sure if that's just something we have to live with or could punt on until we notice such cases.
Sure. Another concern might be the overhead of toggling the bit, so getting this change in sooner than later might actually help direct the development towards implementations where the performance uplift substantially outweighs the cycles spent on managing the DIT state.
To me, it seems unlikely that timing dependent optimizations in privileged code would benefit actual real-world workloads while not resulting in exploitable timing leajs, but user space should be able to manage this however it wants.
IOW, yes. Let's pick a safe default, and when use cases turn up where disabling DIT makes a substantial difference, we can revisit this.
Quoted 18 lines
Quoted 11 lines
Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Mark Rutland <[email protected]>Cc Marc Zyngier <[email protected]>Cc Eric Biggers <[email protected]>Cc Jason A. Donenfeld <[email protected]>Cc Kees Cook <[email protected]>Cc Suzuki K Poulose <[email protected]>Cc Adam Langley <[email protected]>Signed-off-by Ard Biesheuvel <[email protected]>Quoted 6 lines
--- arch/arm64/include/asm/sysreg.h | 3 +++ arch/arm64/kernel/cpufeature.c | 16 ++++++++++++++++ arch/arm64/kernel/entry.S | 3 +++ arch/arm64/tools/cpucaps | 1 + 4 files changed, 23 insertions(+)
Quoted 3 lines
Don't we also need to touch
__cpu_suspend_exit()inarch/am64/kernel/suspend.c? I'm assuming so given that has__uaccess_enable_hw_pan()today.
Indeed, I missed that.
Quoted 2 lines
Presumably we'd also care about this in KVM hyp code?
Presumably, yes but I didn't investigate thoroughly what that would entail. I think this can be managed separately, and I'll look into it once the discussion here converges.
Quoted 31 lines
Quoted 31 lines
arch/arm64/include/asm/sysreg.h +3
-0@@ -94,15 +94,18 @@#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_DIT (x) __emit_inst(0xd500401f | PSTATE_DIT | ((!!x) << PSTATE_Imm_shift))#define SET_PSTATE_TCO (x) __emit_inst(0xd500401f | PSTATE_TCO | ((!!x) << PSTATE_Imm_shift))#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_dit (x) asm volatile(SET_PSTATE_DIT(x))#define set_pstate_ssbs (x) asm volatile(SET_PSTATE_SSBS(x))#define __SYS_BARRIER_INSN (CRm, op2, Rt) \arch/arm64/kernel/cpufeature.c +0
-0@@ -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 ); }
Quoted 9 lines
arch/arm64/kernel/cpufeature.c static void cpu_trap_el0_impdef(const struct arm64_cpu_capabilities *__unused) · line 2083
Quoted 4 lines
arch/arm64/kernel/cpufeature.c +4
-0+static void cpu_enable_dit (const struct arm64_cpu_capabilities *__unused) +{ +set_pstate_dit (1 ); +}I think this wants the same treatment as PAN, i.e. WARN_ON_ONCE(in_interrupt()); with a comment about PSTATE being discareded upon ERET.
This is only called from the CPU feature code, which calls it under stop_machine(), so I decided it was not needed. I don't mind adding it, though, just seems unnecessary to me.
Quoted 102 lines
From: Ard Biesheuvel <[email protected]>
Date: Thu, 27 Oct 2022 15:17:56 +0200Quoted 25 lines · Mark Rutland
Quoted 13 lines · Ard Biesheuvel
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.
Quoted 3 lines
In the kernel, however, we never bothered with DIT: we disable at it boot (i.e.,
INIT_PSTATE_EL1has DIT cleared) and ignore the fact that we might run with DIT enabled if user space happened to set it.FWIW, I did raise this with some architects a while back, and the thinking at the time was that implementations were likely to have data independent timing regardless of the value of the DIT bit, and so manipulating this at exception entry was busy work with no actual gain (especially if we had to handle mismatched big.LITTLE systems).
Quoted 4 lines
Since then, I have become aware of some possible implementation choices which would consider the bit (though I have no idea if anyone is building such implementations).
It's a bit unfortunate that
FEAT_DITis mandatory even if the uarch in question behaves the same regardless. And the fact that it is documented as resetting to0x0, and already being exposed to user space doesn't help either. But that doesn't justify keeping this disabled in the kernel.The architecture does not permit us to distinguish between the cases where this is just busywork and where it does make a difference. So we have to assume it is the latter.
Quoted 11 lines
Quoted 3 lines
Given that 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,
I appreciate this is a simple way to rule out issues of that sort, but I think the "may" in that sentence is doing a lot of work, since:
* IIUC, we don't have a specific case in mind that we're concerned about. I can believe that we think all the crypto code we intend to be constant time is theoretically affected.
I think this reasoning is backwards. I don't think it is necessary to go and identify where we are relying on timing invariance. Crypto keeps coming up, and of course, it is a well known example of where timing variances may leak the plaintext or the key. But crypto is just one way to keep data confidential, and another method we rely on heavily is the privilege boundary between kernel and user space. So just like all crypto should be constant time, all privileged execution should [ideally] be constant time as well.
Quoted 4 lines
* IIUC we haven't gone an audited all constant-time code to check it doesn't happen to use instructions with data-dependent-timing. So there might be more work to do atop this to ensure theoretical correctness.
Sure.
Quoted 4 lines
* AFAIK there are no contemporary implementations where the DIT is both implemented and it being clear results in data-dependent-timing. i.e. we have nothing to actually test on.
Then why on earth are such implementations required to implement
FEAT_DIT??Quoted 4 lines
Given that, it would be nice if we could make this a bit clearer, e.g. state that this is currently a belt-and-braces approach for theoretical cases, rather than an extant side-channel today (as far as we're aware).
Sure - I'll add some extra prose to the commit log to capture the above.
Quoted 10 lines
Quoted 1 line
let's enable DIT while running in the kernel if supported by all CPUs.
FWIW, I think it's reasonable to do this when all CPUs have DIT.
I have a slight fear that (as above) if there are future CPUs which do consider DIT, there's presumably a noticeable performance difference (or the CPU would just provide data-independent-timing regardless), but I'm not sure if that's just something we have to live with or could punt on until we notice such cases.
Sure. Another concern might be the overhead of toggling the bit, so getting this change in sooner than later might actually help direct the development towards implementations where the performance uplift substantially outweighs the cycles spent on managing the DIT state.
Quoted 7 lines
To me, it seems unlikely that timing dependent optimizations in privileged code would benefit actual real-world workloads while not resulting in exploitable timing leajs, but user space should be able to manage this however it wants.
IOW, yes. Let's pick a safe default, and when use cases turn up where disabling DIT makes a substantial difference, we can revisit this.
FWIW, I came to the same conclusion and that's what I implemented for OpenBSD.
Quoted 81 lines
Quoted 18 lines
Quoted 11 lines
Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Mark Rutland <[email protected]>Cc Marc Zyngier <[email protected]>Cc Eric Biggers <[email protected]>Cc Jason A. Donenfeld <[email protected]>Cc Kees Cook <[email protected]>Cc Suzuki K Poulose <[email protected]>Cc Adam Langley <[email protected]>Signed-off-by Ard Biesheuvel <[email protected]>Quoted 6 lines
--- arch/arm64/include/asm/sysreg.h | 3 +++ arch/arm64/kernel/cpufeature.c | 16 ++++++++++++++++ arch/arm64/kernel/entry.S | 3 +++ arch/arm64/tools/cpucaps | 1 + 4 files changed, 23 insertions(+)Quoted 3 lines
Don't we also need to touch
__cpu_suspend_exit()inarch/am64/kernel/suspend.c? I'm assuming so given that has__uaccess_enable_hw_pan()today.Indeed, I missed that.
Quoted 2 lines
Presumably we'd also care about this in KVM hyp code?
Presumably, yes but I didn't investigate thoroughly what that would entail. I think this can be managed separately, and I'll look into it once the discussion here converges.
Quoted 31 lines
Quoted 31 lines
arch/arm64/include/asm/sysreg.h +3
-0@@ -94,15 +94,18 @@#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_DIT (x) __emit_inst(0xd500401f | PSTATE_DIT | ((!!x) << PSTATE_Imm_shift))#define SET_PSTATE_TCO (x) __emit_inst(0xd500401f | PSTATE_TCO | ((!!x) << PSTATE_Imm_shift))#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_dit (x) asm volatile(SET_PSTATE_DIT(x))#define set_pstate_ssbs (x) asm volatile(SET_PSTATE_SSBS(x))#define __SYS_BARRIER_INSN (CRm, op2, Rt) \arch/arm64/kernel/cpufeature.c +0
-0@@ -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 ); }Quoted 9 lines
arch/arm64/kernel/cpufeature.c static void cpu_trap_el0_impdef(const struct arm64_cpu_capabilities *__unused) · line 2083
Quoted 4 lines
arch/arm64/kernel/cpufeature.c +4
-0+static void cpu_enable_dit (const struct arm64_cpu_capabilities *__unused) +{ +set_pstate_dit (1 ); +}I think this wants the same treatment as PAN, i.e. WARN_ON_ONCE(in_interrupt()); with a comment about PSTATE being discareded upon ERET.
This is only called from the CPU feature code, which calls it under
stop_machine(), so I decided it was not needed. I don't mind adding it, though, just seems unnecessary to me._______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Ard,
Quoted 4 lines · Ard Biesheuvel
Given that 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, let's enable DIT while running in the kernel if supported by all CPUs.
This patch looks good to me, though I'm not an expert in low-level arm64 stuff. It's a bit unfortunate that we have to manually create the .inst to enable DIT instead of just using the assembler. But it looks like there's a reason for it (it's done for PAN et al. too), and based on the manual it looks correct.
Two nits below:
Quoted 11 lines
arch/arm64/include/asm/sysreg.h +1
-0@@ -94,15 +94,18 @@#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)
arch/arm64/include/asm/sysreg.h line 109
Quoted 10 lines
arch/arm64/include/asm/sysreg.h +2
-0#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_DIT (x) __emit_inst(0xd500401f | PSTATE_DIT | ((!!x) << PSTATE_Imm_shift))#define SET_PSTATE_TCO (x) __emit_inst(0xd500401f | PSTATE_TCO | ((!!x) << PSTATE_Imm_shift))#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_dit (x) asm volatile(SET_PSTATE_DIT(x))#define set_pstate_ssbs (x) asm volatile(SET_PSTATE_SSBS(x))
To keep the order consistent, set_pstate_dit() should be defined after set_pstate_ssbs(), not before.
Quoted 8 lines
arch/arm64/include/asm/sysreg.h +0
-0/* Internal helper functions to match cpu capability type */ static bool
cpucap_late_cpu_optional(const struct arm64_cpu_capabilities *cap)arch/arm64/include/asm/sysreg.h +1
-0@@ -2640,6 +2645,17 @@ static const struct arm64_cpu_capabilities arm64_features[] = { .matches = has_cpuid_feature, .cpu_enable = cpu_trap_el0_impdef, }, + {
arch/arm64/include/asm/sysreg.h static const struct arm64_cpu_capabilities arm64_features[] = { · line 2658
Quoted 10 lines
arch/arm64/include/asm/sysreg.h +10
-0+ .desc = "Data independent timing control (DIT)", + .capability = ARM64_HAS_DIT, + .type = ARM64_CPUCAP_SYSTEM_FEATURE, + .matches = has_cpuid_feature, + .sys_reg = SYS_ID_AA64PFR0_EL1, + .field_pos = ID_AA64PFR0_EL1_DIT_SHIFT, + .field_width = 4, + .min_field_value = 1, + .cpu_enable = cpu_enable_dit, + },
The other entries in this array are explicit about '.sign = FTR_UNSIGNED' (even though FTR_UNSIGNED is defined to false, so it's the default value).
- Eric
Quoted 9 lines · Eric Biggers
Hi Ard,
Quoted 4 lines · Ard Biesheuvel
Given that 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, let's enable DIT while running in the kernel if supported by all CPUs.
Quoted 5 lines
This patch looks good to me, though I'm not an expert in low-level arm64 stuff. It's a bit unfortunate that we have to manually create the .inst to enable DIT instead of just using the assembler. But it looks like there's a reason for it (it's done for PAN et al. too), and based on the manual it looks correct.
Yes. The reason is that the assembler requires -march=armv8.2-a to be passed when using the DIT register (and similar requirements apply to the other registers). However, doing so may result in object code that can no longer run on pre-v8.2 cores, whereas the DIT accesses themselves are only emitted in a carefully controlled manner anyway, so keeping the arch baseline to v8.0 and using .inst is the cleanest way around this.
Quoted 19 lines
Two nits below:
Quoted 17 lines
arch/arm64/include/asm/sysreg.h +2
-0@@ -94,15 +94,18 @@#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_DIT (x) __emit_inst(0xd500401f | PSTATE_DIT | ((!!x) << PSTATE_Imm_shift))#define SET_PSTATE_TCO (x) __emit_inst(0xd500401f | PSTATE_TCO | ((!!x) << PSTATE_Imm_shift))
Quoted 8 lines
arch/arm64/include/asm/sysreg.h line 109
Quoted 4 lines
arch/arm64/include/asm/sysreg.h +1
-0#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_dit (x) asm volatile(SET_PSTATE_DIT(x))#define set_pstate_ssbs (x) asm volatile(SET_PSTATE_SSBS(x))To keep the order consistent,
set_pstate_dit()should be defined afterset_pstate_ssbs(), not before.
Ack. Seems I just inserted it one from the bottom without actually reading :-)
Quoted 13 lines
Quoted 13 lines
arch/arm64/include/asm/sysreg.h +0
-0/* Internal helper functions to match cpu capability type */ static bool
cpucap_late_cpu_optional(const struct arm64_cpu_capabilities *cap)arch/arm64/include/asm/sysreg.h +6
-0@@ -2640,6 +2645,17 @@ 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, + .matches = has_cpuid_feature, + .sys_reg = SYS_ID_AA64PFR0_EL1,
Quoted 9 lines
arch/arm64/include/asm/sysreg.h static const struct arm64_cpu_capabilities arm64_features[] = { · line 2658
Quoted 5 lines
arch/arm64/include/asm/sysreg.h +5
-0+ .field_pos = ID_AA64PFR0_EL1_DIT_SHIFT, + .field_width = 4, + .min_field_value = 1, + .cpu_enable = cpu_enable_dit, + },The other entries in this array are explicit about '.sign =
FTR_UNSIGNED' (even thoughFTR_UNSIGNEDis defined to false, so it's the default value).
Ack.
Catalin Marinas
maintainerQuoted 12 lines · Ard Biesheuvel
Quoted 10 lines · Eric Biggers
Quoted 4 lines · Ard Biesheuvel
Given that 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, let's enable DIT while running in the kernel if supported by all CPUs.
This patch looks good to me, though I'm not an expert in low-level arm64 stuff. It's a bit unfortunate that we have to manually create the .inst to enable DIT instead of just using the assembler. But it looks like there's a reason for it (it's done for PAN et al. too), and based on the manual it looks correct.
Quoted 7 lines
Yes. The reason is that the assembler requires -march=armv8.2-a to be passed when using the DIT register (and similar requirements apply to the other registers). However, doing so may result in object code that can no longer run on pre-v8.2 cores, whereas the DIT accesses themselves are only emitted in a carefully controlled manner anyway, so keeping the arch baseline to v8.0 and using .inst is the cleanest way around this.
We worked around this already by defining asm-arch in arch/arm64/Makefile to the latest that the assembler supports while keeping the C compiler on armv8.0. Unlike the C compiler, the assembler shouldn't generate new instructions unless specifically asked through inline asm or .S files. We use this trick for MTE already (and LSE atomics), though we needed another __MTE_PREAMBLE as armv8.5-a wasn't sufficient for these instructions.
I think we ended up with .inst initially as binutils did not support some of those instructions. We could try to clean them up but it's a bit of a hassle to check which versions your binutils supports.
Signature · 1 line
Catalin
Quoted 30 lines · Catalin Marinas
Quoted 12 lines · Ard Biesheuvel
Quoted 10 lines · Eric Biggers
Quoted 4 lines · Ard Biesheuvel
Given that 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, let's enable DIT while running in the kernel if supported by all CPUs.
This patch looks good to me, though I'm not an expert in low-level arm64 stuff. It's a bit unfortunate that we have to manually create the .inst to enable DIT instead of just using the assembler. But it looks like there's a reason for it (it's done for PAN et al. too), and based on the manual it looks correct.
Quoted 7 lines
Yes. The reason is that the assembler requires -march=armv8.2-a to be passed when using the DIT register (and similar requirements apply to the other registers). However, doing so may result in object code that can no longer run on pre-v8.2 cores, whereas the DIT accesses themselves are only emitted in a carefully controlled manner anyway, so keeping the arch baseline to v8.0 and using .inst is the cleanest way around this.
We worked around this already by defining asm-arch in
arch/arm64/Makefileto the latest that the assembler supports while keeping the C compiler on armv8.0. Unlike the C compiler, the assembler shouldn't generate new instructions unless specifically asked through inline asm or .S files. We use this trick for MTE already (and LSE atomics), though we needed another__MTE_PREAMBLEas armv8.5-a wasn't sufficient for these instructions.
Quoted 4 lines
I think we ended up with .inst initially as binutils did not support some of those instructions. We could try to clean them up but it's a bit of a hassle to check which versions your binutils supports.
OK, good to know.
However, I double checked, and DIT needs v8.4-a (not v8.2 as i mentioned above), and my ubuntu 16.04 toolchain, which has GCC 5.3, only goes up to v8.2
So I guess we should be able to fix this at /some/ point, but for now, I'll need to stick with the __inst()
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel