aarch64-dit crate is missing fencing (for both compiler and CPU)
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 674
- Forks
- 170
- Avg merge
- 1d 12h
- Merged PRs (30d)
- 10
Description
The aarch64-dit crate has two notable flaws:
- It is possible for the compiler to reorder operations outside the critical section where DIT is enabled.
- It does not perform speculation fencing, which is documented as necessary at least on apple platforms in this document: https://developer.apple.com/documentation/xcode/writing-arm64-code-for-apple-platforms#Enable-DIT-for-constant-time-cryptographic-operations.
Flaw 1: Compiler Reordering
Here's an example of the first flaw: https://godbolt.org/z/8fdsnxrc5. Essentially the compiler is free to move instructions outside of the asm!("msr DIT #1") and asm!("msr DIT #0") guarded region, and you can see that it essentially compiles:
if have_dit {
dit_enable();
}
let res = sensitive(a, b);
if have_dit {
dit_disable();
}
return res;
into
if have_dit {
dit_enable();
dit_disable();
}
return sensitive(a, b);
which defeats the point, since the instructions which are data-dependent are no longer in the block where DIT is enabled.
In an ideal world, inserting compiler fences (or atomic fences, or adjusting the asm! options...) would fix this, however they only apply to memory accesses, so that does not work. The only approach I can think of that does work is to use an indirect jump whose target we obscure from the vision of the compiler. Something like:
impl Dit {
// ...snip ...
/// Call `f` with DIT enabled, returning the result. Takes care to ensure
/// that that any computations which occur during `f` do not leak outside
/// the critical section.
pub fn with<R, F: FnOnce() -> R>(s: &self, f: F) -> R {
let guard = self.enable();
let mut o: Option<F> = Some(f);
// Note: Could be unwrap_unchecked().
let f: &mut dyn FnMut() -> R = &mut || o.take().unwrap()();
core::hint::black_box(f)()
}
// ...snip...
}
This works, as demonstrated here: https://godbolt.org/z/fv1Gofjxc (note: the add instruction was changed to floating point addition just so that it's clearer in the assembly listing what is happening—you can see that the fadd takes place in the indirectly called function, which is called (the blr x8) between the msr DIT #1; and msr DIT #0 instructions.
Flaw 2: Speculation
I'm unsure if this is needed on targets besides apple (my suspicion is yes, or at least that there's no guarantee for it to be a no), but apple documents it as necessary in the linked documentation.
Essentially, you need something like this after you enable DIT, and before you disable it (actually, I think you might not need it before disabling it, since it's fine if a DIT-mode processor speculates operations outside of DIT mode).
if is_aarch64_feature_detected!("sb") {
// note: in the real impl this must happen
// in a `#[target_feature(enable = "sb")]` function)
asm!("sb");
} else {
asm!("dsb nsh", "isb sy");
}
Of course, a real impl needs to:
- Put
asm!("sb")inside a#[target_feature(enable = "sb")]function. - Use
cpufeaturesrather thanis_aarch64_feature_detected(which requires addingsbsupport tocpufeatures). - And so on.
A patch fixing these issues is here: https://github.com/RustCrypto/utils/compare/master...thomcc:rust-crypto-utils:aarch64-dit-sb-fix. I don't have time to do a PR and respond to review feedback, feel free to mess with it however you please, or to take a different approach that solves the same problem.
One issue with my patch is that it leaves the Dit::enable() function present and non-deprecated, even though it's a footgun. I think probably the right call would be to remove it. But that begs the question of whether or not the API should have a different structure in other ways too.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by inspecting the aarch64-dit crate and the linked Godbolt examples to verify that compiler reordering can move sensitive operations outside the DIT region. Review the linked Apple guidance and proposed comparison patch for speculation fencing and cpufeatures support. Done means both compiler and CPU speculation are fenced correctly, with the enable API and its safety implications resolved.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- cryptography, security
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100