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

Conversation

swenson
Copy link

@swenson swenson commented Oct 10, 2024

cfi_launder is meant to prevent the Rust compiler from optimizing a value away.

Our current implementation uses core::hint::black_box(), which is the recommended way in Rust. The problem is, this appears to often force the argument to spill into memory and to be reloaded, which can be a lot of extra instructions.

The original inspiration for this function is from, I believe, OpenTitan's launder* functions. There, they use an LLVM-specific trick of a blank inline assembly block to force the compiler to keep the argument in a register.

After reviewing our code and speaking with @vsonims, it sounds like the intention of the launder in our code is to prevent the compiler from optimizing the value away (as the comments suggest), so the simpler inline assembly trick may be sufficient (since we use the official Rust compiler, which uses LLVM).

The biggest problem is that we launder many types of values in our code and not all of them fit into a register.

So, this PR represents an incremental change: for u32s and similar small types, we implement cfi_launder using the inline assembly trick from OpenTitan. For any other types, we have a trait that can be derived that will call core::hint::black_box in the same way as today.

We can do future follow-up PRs to try to try to clean up some of those other uses of cfi_launder to hopefully shrink the code more.

I also slipped in avoid a few extra copies in the verifier by using references instead of copies (this saves ~80 bytes of instruction space).

This PR appears to shrink the ROM code size by 1232 bytes and the runtime firmware by 700 bytes.

} 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: u32) -> u32 {
let mut val = val;
unsafe {
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.

`cfi_launder` is meant to prevent the Rust compiler from optimizing a
value away.

Our current implementation uses `core::hint::black_box()`, which is the
recommended way in Rust. The problem is, this appears to often force the
argument to spill into memory and to be reloaded, which can be a lot of
extra instructions.

The original inspiration for this function is from, I believe, [OpenTitan's launder*
functions](https://github.com/lowRISC/opentitan/blob/master/sw/device/lib/base/hardened.h#L193).
There, they use an LLVM-specific trick of a blank inline assembly block
to force the compiler to keep the argument in a register.

After reviewing our code and speaking with @vsonims, it sounds like the
intention of the launder in our code is to prevent the compiler from
optimizing the value away (as the comments suggest), so the simpler
inline assembly trick may be sufficient (since we use the official
Rust compiler, which uses LLVM).

The biggest problem is that we launder many types of values in our code
and not all of them fit into a register.

So, this PR represents an incremental change: for `u32`s and similar
small types, we implement `cfi_launder` using the inline assembly trick
from OpenTitan. For any other types, we have a trait that can be derived
that will call `core::hint::black_box` in the same way as today.

We can do future follow-up PRs to try to try to clean up some of those
other uses of `cfi_launder` to hopefully shrink the code more.

This PR appears to shrink the ROM code size by 1152 bytes and the runtime
firmware by 616 bytes.
This saves an additional 80 and 84 bytes in the ROM and runtime,
respectively.
jhand2
jhand2 previously approved these changes Oct 14, 2024
@swenson
Copy link
Author

swenson commented Oct 14, 2024

(I've also looked at removing Copy traits from a few other types and trying to use more references so that the laundering could be more effective without copying, but I was surprised to find that this increased the code size.

There are more likely a few more types though that would be worth streamlining so we can save more code space, but I leave that to a future PR.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants