Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cfi: Simpler launder implementation for common types #1714

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

78 changes: 78 additions & 0 deletions cfi/derive/src/cfi_asm_test.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
// Licensed under the Apache-2.0 license

// These tests are here so that they are excluded in FPGA tests.

// These tests don't directly import the CFI code. If they fail,
// this likely indicates that the CFI laundering code may not
// be doing what we want, and we need to investigate.

#[cfg(test)]
mod test {

const START: &str = "
#![no_std]

pub fn add(mut a: u32, mut b: u32) -> u32 {
launder(a) + launder(a) + launder(b) + launder(b)
}
";

const LAUNDER: &str = "
#[inline(always)]
fn launder(mut val: u32) -> u32 {
// Safety: this is a no-op, since we don't modify the input.
unsafe {
core::arch::asm!(
\"/* {t} */\",
t = inout(reg) val,
);
}
val
}";

const NO_LAUNDER: &str = "
#[inline(always)]
fn launder(mut val: u32) -> u32 {
val
}
";

fn compile_to_riscv32_asm(src: String) -> String {
let dir = std::env::temp_dir();
let src_path = dir.join("asm.rs");
let dst_path = dir.join("asm.s");

std::fs::write(src_path.clone(), src).expect("could not write asm file");

let p = std::process::Command::new("rustc")
.args([
"--crate-type=lib",
"--target",
"riscv32imc-unknown-none-elf",
"-C",
"opt-level=s",
"--emit",
"asm",
src_path.to_str().expect("could not convert path"),
"-o",
dst_path.to_str().expect("could not convert path"),
])
.output()
.expect("failed to compile");
assert!(p.status.success());
std::fs::read_to_string(dst_path).expect("could not read asm file")
}

#[test]
fn test_launder() {
// With no laundering, LLVM can simplify the double add to a shift left.
let src = format!("{}{}", START, NO_LAUNDER);
let asm = compile_to_riscv32_asm(src);
assert!(asm.contains("sll"));

// With laundering, LLVM cannot simplify the double add and has to use the register twice.
let src = format!("{}{}", START, LAUNDER);
let asm = compile_to_riscv32_asm(src);
assert!(!asm.contains("sll"));
}
}
15 changes: 15 additions & 0 deletions cfi/derive/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,14 @@ References:

--*/

mod cfi_asm_test;

use proc_macro::TokenStream;
use quote::{format_ident, quote, ToTokens};
use syn::__private::TokenStream2;
use syn::parse_macro_input;
use syn::parse_quote;
use syn::DeriveInput;
use syn::FnArg;
use syn::ItemFn;

Expand Down Expand Up @@ -94,3 +97,15 @@ fn cfi_fn(mod_fn: bool, input: TokenStream) -> TokenStream {

code.into()
}

#[proc_macro_derive(Launder)]
pub fn derive_launder_trait(input: TokenStream) -> TokenStream {
let input = parse_macro_input!(input as DeriveInput);
let name = input.ident;
let (impl_generics, ty_generics, _) = input.generics.split_for_impl();
let expanded = quote! {
impl #impl_generics caliptra_cfi_lib::LaunderTrait<#name #ty_generics> for caliptra_cfi_lib::Launder<#name #ty_generics> {}
impl #impl_generics caliptra_cfi_lib::LaunderTrait<&#name #ty_generics> for caliptra_cfi_lib::Launder<&#name #ty_generics> {}
};
TokenStream::from(expanded)
}
122 changes: 116 additions & 6 deletions cfi/lib/src/cfi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use caliptra_error::CaliptraError;
use crate::CfiCounter;
use core::cfg;
use core::cmp::{Eq, Ord, PartialEq, PartialOrd};
use core::marker::Copy;
use core::marker::{Copy, PhantomData};

/// CFI Panic Information
#[derive(Debug, Clone, Copy, Eq, PartialEq)]
Expand Down Expand Up @@ -94,16 +94,107 @@ impl From<CfiPanicInfo> for CaliptraError {
/// # Returns
///
/// `T` - Same value
pub fn cfi_launder<T>(val: T) -> T {
pub fn cfi_launder<T>(val: T) -> T
where
Launder<T>: LaunderTrait<T>,
{
if cfg!(feature = "cfi") {
// Note: The black box seems to be disabling more optimization
// than necessary and results in larger binary size
core::hint::black_box(val)
Launder { _val: PhantomData }.launder(val)
} else {
val
}
}

pub trait LaunderTrait<T> {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These Rust type acrobatics are so that we can have special, smaller implementations for register-sized but default to core::hint::black_box for everything else.

I'm open to suggestions on how to make this simpler or avoid a derive.

fn launder(&self, val: T) -> T {
core::hint::black_box(val)
}
}

pub struct Launder<T> {
_val: PhantomData<T>,
}

// Inline-assembly laundering trick is adapted from OpenTitan:
// https://github.com/lowRISC/opentitan/blob/master/sw/device/lib/base/hardened.h#L193
//
// NOTE: This implementation is LLVM-specific, and should be considered to be
// a no-op in every other compiler. For example, GCC has in the past peered
// into the insides of assembly blocks.
//
// At the time of writing, it seems preferable to have something we know is
// correct rather than being overly clever; this is recorded here in case
// the current implementation is unsuitable and we need something more
// carefully tuned.
//
// Unlike in C, we don't have volatile assembly blocks, so this doesn't
// necessarily prevent reordering by LLVM.
//
// When we're building for static analysis, reduce false positives by
// short-circuiting the inline assembly block.
impl LaunderTrait<u32> for Launder<u32> {
#[allow(asm_sub_register)]
fn launder(&self, val: u32) -> u32 {
let mut val = val;
// Safety: this is a no-op, since we don't modify the input.
unsafe {
// We use inout so that LLVM thinks the value might
// be mutated by the assembly and can't eliminate it.
core::arch::asm!(
Copy link
Collaborator

@jhand2 jhand2 Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is magic enough to be worth a comment about why it works 😄 Also can we be confident that this will continue to work in future versions of LLVM? I wonder if we can write a test that proves the compiled binary didn't optimizer out values passed to cfi_launder.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll definitely write a comment.

It's quite difficult to test this, I think, and it certainly is not guaranteed by LLVM in the future (nor would be basically any trick, probably, since all of these are meant to fool the compiler without actually doing anything). I verified it by loading code into Compiler Explorer for now.

There are some ways to test this by invoking the compiler in a test and checking the emitted assembly and, for example, counting instructions with and without this function being applied.

If this stops working in the future, this doesn't stop the code using it from working, it just means that one layer of protection is removed. And theoretically we'd notice because the code size would change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test that invokes rustc --emit asm and does a simple check if a functions is optimized enough (with and without laundering).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice :) Thanks!

I talked with the OT folks and they do manual pre-TO validation for things like this. Might be worth doing something at ROM release time (or adding a note to pre-TO guidance for integrators) to spot check important sections.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're mostly safe here, since we pin the version of Cargo (and presumably rustc and LLVM). So we mostly need to validate when we update our toolchain.

"/* {t} */",
t = inout(reg) val,
);
}
val
}
}

impl LaunderTrait<bool> for Launder<bool> {
#[allow(asm_sub_register)]
fn launder(&self, val: bool) -> bool {
let mut val = val as u32;
// Safety: this is a no-op, since we don't modify the input.
unsafe {
core::arch::asm!(
"/* {t} */",
t = inout(reg) val,
);
}
val != 0
}
}

impl LaunderTrait<usize> for Launder<usize> {
#[allow(asm_sub_register)]
fn launder(&self, mut val: usize) -> usize {
// Safety: this is a no-op, since we don't modify the input.
unsafe {
core::arch::asm!(
"/* {t} */",
t = inout(reg) val,
);
}
val
}
}

impl<const N: usize, T> LaunderTrait<[T; N]> for Launder<[T; N]> {}
impl<'a, const N: usize, T> LaunderTrait<&'a [T; N]> for Launder<&'a [T; N]> {
fn launder(&self, val: &'a [T; N]) -> &'a [T; N] {
let mut valp = val.as_ptr() as *const [T; N];
// Safety: this is a no-op, since we don't modify the input.
unsafe {
core::arch::asm!(
"/* {t} */",
t = inout(reg) valp,
);
&*valp
}
}
}
impl LaunderTrait<Option<u32>> for Launder<Option<u32>> {}
impl LaunderTrait<CfiPanicInfo> for Launder<CfiPanicInfo> {}

/// Control flow integrity panic
///
/// This panic is raised when the control flow integrity error is detected
Expand Down Expand Up @@ -157,6 +248,7 @@ macro_rules! cfi_assert_macro {
pub fn $name<T>(lhs: T, rhs: T)
where
T: $trait1 + $trait2,
Launder<T>: LaunderTrait<T>,
{
if cfg!(feature = "cfi") {
CfiCounter::delay();
Expand Down Expand Up @@ -184,10 +276,28 @@ cfi_assert_macro!(cfi_assert_lt, <, Ord, PartialOrd, AssertLtFail);
cfi_assert_macro!(cfi_assert_ge, >=, Ord, PartialOrd, AssertGeFail);
cfi_assert_macro!(cfi_assert_le, <=, Ord, PartialOrd, AssertLeFail);

// special case for bool assert
#[inline(always)]
#[allow(unused)]
pub fn cfi_assert_bool(cond: bool) {
if cfg!(feature = "cfi") {
CfiCounter::delay();
if !cond {
cfi_panic(CfiPanicInfo::AssertEqFail);
}

// Second check for glitch protection
CfiCounter::delay();
if !cfi_launder(cond) {
cfi_panic(CfiPanicInfo::AssertEqFail);
}
}
}

#[macro_export]
macro_rules! cfi_assert {
($cond: expr) => {
cfi_assert_eq($cond, true)
cfi_assert_bool($cond)
};
}

Expand Down
3 changes: 2 additions & 1 deletion drivers/src/array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ Abstract:

--*/

use caliptra_cfi_derive::Launder;
use core::mem::MaybeUninit;
use zerocopy::{AsBytes, FromBytes};
use zeroize::Zeroize;
Expand All @@ -26,7 +27,7 @@ macro_rules! static_assert {
/// The `Array4xN` type represents large arrays in the native format of the Caliptra
/// cryptographic hardware, and provides From traits for converting to/from byte arrays.
#[repr(transparent)]
#[derive(Debug, Clone, Copy, PartialEq, Eq, Zeroize)]
#[derive(Debug, Clone, Copy, PartialEq, Eq, Launder, Zeroize)]
pub struct Array4xN<const W: usize, const B: usize>(pub [u32; W]);
impl<const W: usize, const B: usize> Array4xN<W, B> {
pub const fn new(val: [u32; W]) -> Self {
Expand Down
7 changes: 4 additions & 3 deletions drivers/src/fuse_bank.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ Abstract:
--*/

use crate::Array4x12;
use caliptra_cfi_derive::Launder;
use caliptra_registers::soc_ifc::SocIfcReg;
use zerocopy::AsBytes;

Expand All @@ -33,7 +34,7 @@ pub enum X509KeyIdAlgo {
}

bitflags::bitflags! {
#[derive(Default, Copy, Clone, Debug)]
#[derive(Default, Copy, Clone, Debug, Launder)]
pub struct VendorPubKeyRevocation : u32 {
const KEY0 = 0b0001;
const KEY1 = 0b0010;
Expand Down Expand Up @@ -76,7 +77,7 @@ impl FuseBank<'_> {
/// * None
///
/// # Returns
/// key id crypto algorithm
/// key id crypto algorithm
///
pub fn idev_id_x509_key_id_algo(&self) -> X509KeyIdAlgo {
let soc_ifc_regs = self.soc_ifc.regs();
Expand All @@ -101,7 +102,7 @@ impl FuseBank<'_> {
/// * None
///
/// # Returns
/// manufacturer serial number
/// manufacturer serial number
///
pub fn ueid(&self) -> [u8; 17] {
let soc_ifc_regs = self.soc_ifc.regs();
Expand Down
3 changes: 2 additions & 1 deletion drivers/src/soc_ifc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ Abstract:

--*/

use caliptra_cfi_derive::Launder;
use caliptra_error::{CaliptraError, CaliptraResult};
use caliptra_registers::soc_ifc::enums::DeviceLifecycleE;
use caliptra_registers::soc_ifc::{self, SocIfcReg};
Expand Down Expand Up @@ -382,7 +383,7 @@ impl From<u32> for MfgFlags {
}

/// Reset Reason
#[derive(Debug, Eq, PartialEq, Copy, Clone)]
#[derive(Debug, Eq, PartialEq, Copy, Clone, Launder)]
pub enum ResetReason {
/// Cold Reset
ColdReset,
Expand Down
3 changes: 1 addition & 2 deletions fmc/src/flow/rt_alias.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@ Abstract:

--*/
use caliptra_cfi_derive::cfi_impl_fn;
use caliptra_cfi_lib::cfi_assert_eq;
use caliptra_cfi_lib::{cfi_assert, cfi_launder};
use caliptra_cfi_lib::{cfi_assert, cfi_assert_bool, cfi_assert_eq, cfi_launder};

use crate::flow::crypto::Crypto;
use crate::flow::dice::{DiceInput, DiceOutput};
Expand Down
2 changes: 2 additions & 0 deletions image/types/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ doctest = false

[dependencies]
arbitrary = { workspace = true, optional = true }
caliptra-cfi-derive.workspace = true
caliptra-cfi-lib.workspace = true
caliptra-lms-types.workspace = true
memoffset.workspace = true
zerocopy.workspace = true
Expand Down
Loading
Loading