cmse: clear padding when crossing the secure boundary#157397
Conversation
| // FIXME: dedup with the one in | ||
| // `compiler/rustc_const_eval/src/interpret/validity.rs` | ||
| /// Represents a set of `Size` values as a sorted list of ranges. | ||
| // These are (offset, length) pairs, and they are sorted and mutually disjoint, | ||
| // and never adjacent (i.e. there's always a gap between two of them). | ||
| #[derive(Debug, Clone)] | ||
| struct RangeSet(Vec<(Size, Size)>); |
There was a problem hiding this comment.
@RalfJung given the dependency graph, do you have objections to moving the rustc_const_eval into rustc_abi?
There was a problem hiding this comment.
moving the rustc_const_eval into rustc_abi?
... you want to move the entire crate into rustc_abi? I doubt that's what you mean. I am confused.^^
There was a problem hiding this comment.
If you mean moving this data structure -- if it gets moved it should move to rustc_data_structures.
There was a problem hiding this comment.
It can't due to the use of the Size type which is defined in rustc_abi.
There was a problem hiding this comment.
It should probably be generic over some traits then.
There was a problem hiding this comment.
Hmm, I've found something that kind of works, but rustc_const_eval uses a bunch of implementation details.
There was a problem hiding this comment.
rustc_const_eval uses a bunch of implementation details.
Is that why the field is pub?
There was a problem hiding this comment.
Yeah, in particular the const eval code breaks the invariant of the data structure by adding an empty range to make some other logic work out.
| /// Ranges of bytes that are initialized for some valid value of this type. In particular for | ||
| /// enums and unions there are offsets that are initialized for some variants but not for | ||
| /// others. | ||
| fn add_data_ranges<C>(self, cx: &C, base_offset: Size, out: &mut RangeSet) |
There was a problem hiding this comment.
This sounds very strange. You can't just zero all bytes that would be allowed to be uninit -- that can introduce UB as it might zero something where the active enum variant says it must be non-zero.
There was a problem hiding this comment.
Maybe the text is not clear, but this function finds the union of all fields, not the intersection.
There was a problem hiding this comment.
Specifically for e.g.
// size_of::<X>() == 8
#[repr(C, align(4))]
enum X {
A(u8),
B(u16),
}
I believe the discriminant is 32 bits here, then one variant uses 1 byte, the other 2, and this function will count the first 6 bytes as data bytes. The remaining 2 bytes are padding and should be zeroed.
There was a problem hiding this comment.
I see, makes sense.
Well, kinda. Obviously the enum might be in variant A and then there's still a byte of junk being passed around here. So I hope you don't intend for this to be used for anything critical.
The entire concept of "clearing padding for security" sounds pretty backwards to me. Similar to these crates that try to "erase" secrets from memory. It's trying to shove a square into a round hole.
There was a problem hiding this comment.
Well, kinda. Obviously the enum might be in variant A and then there's still a byte of junk being passed around here. So I hope you don't intend for this to be used for anything critical.
The lint in #147697 will warn on this sort of case. That PR will need some updating though after this one is merged.
There was a problem hiding this comment.
That entire approach feels like piling hacks on top of hacks. I guess that's what we have to do if we want to copy what C does.
This comment has been minimized.
This comment has been minimized.
cb1d784 to
3be4bee
Compare
3be4bee to
a037297
Compare
There was a problem hiding this comment.
r=me if @RalfJung is happy their comments are resolved
|
In that case, r? RalfJung |
|
|
|
|
||
| if self.fn_abi.conv == CanonAbi::Arm(ArmCall::CCmseNonSecureEntry) { | ||
| // The return value of an `extern "cmse-nonsecure-entry"` function crosses the secure | ||
| // boundary. Zero padding bytes so information does not leak. |
There was a problem hiding this comment.
This should at least acknowledge that value-dependent padding is ignored, and reference somewhere that explains why that is okay.
| ); | ||
|
|
||
| // The arguments of an `extern "cmse-nonsecure-call"` function cross the secure | ||
| // boundary. Zero padding bytes so information does not leak. |
There was a problem hiding this comment.
This should at least acknowledge that value-dependent padding is ignored, and reference somewhere that explains why that is okay.
25dc638 to
5f34c50
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@bors r=davidtwco,RalfJung |
…avidtwco,RalfJung cmse: clear padding when crossing the secure boundary tracking issue: rust-lang#81391 tracking issue: rust-lang#75835 RFC: rust-lang/rfcs#3884 related: rust-lang#147697 quick context: cmse creates a distinction between code running in secure mode and non-secure mode (think kernel space versus user space). Secure mode has access to data (e.g. encryption keys) that must not leak to non-secure mode. They use a special calling convention that clears unused registers, but padding in arguments/return values can contain stale secure data. This PR clears the padding bytes (and similar, e.g. space not used in any variant of a union/enum) when values are passed over the secure boundary. Separately we'll have a lint to warn on enums and unions being passed across the boundary: for them we can't statically know whether the variant that is passed contains padding. This is conceptually modeled after a similar feature in `clang` ([implementation](https://github.com/llvm/llvm-project/blob/065a39b9f7f06fca0926394096ee1c1fac41d446/clang/lib/CodeGen/CGCall.cpp#L4041-L4087)). cc @Jules-Bertholet r? @davidtwco
Rollup of 10 pull requests Successful merges: - #158410 (Update LLVM for Mach-O __LINKEDIT alignment fix.) - #157397 (cmse: clear padding when crossing the secure boundary) - #158036 (Add -Zinstrument-mcount=fentry to -Zinstrument-mcount) - #158330 (llvm: use intrinsics for f16, f32 minimum/maximum) - #158359 (fix(tests): allow either branch direction in ilog_known_base) - #158067 (LLVM 23: Adapt codegen test to moved assume) - #158261 (Move part of the target checking for `#[may_dangle]` to the parser) - #158358 (Fix invalid E0609 raw pointer deref suggestion inside macros) - #158392 (delegation: add tests for defaults and infers in generics) - #158394 (Generate synthetic generic args only for delegation's child segment)
Rollup merge of #157397 - folkertdev:cmse-clear-padding, r=davidtwco,RalfJung cmse: clear padding when crossing the secure boundary tracking issue: #81391 tracking issue: #75835 RFC: rust-lang/rfcs#3884 related: #147697 quick context: cmse creates a distinction between code running in secure mode and non-secure mode (think kernel space versus user space). Secure mode has access to data (e.g. encryption keys) that must not leak to non-secure mode. They use a special calling convention that clears unused registers, but padding in arguments/return values can contain stale secure data. This PR clears the padding bytes (and similar, e.g. space not used in any variant of a union/enum) when values are passed over the secure boundary. Separately we'll have a lint to warn on enums and unions being passed across the boundary: for them we can't statically know whether the variant that is passed contains padding. This is conceptually modeled after a similar feature in `clang` ([implementation](https://github.com/llvm/llvm-project/blob/065a39b9f7f06fca0926394096ee1c1fac41d446/clang/lib/CodeGen/CGCall.cpp#L4041-L4087)). cc @Jules-Bertholet r? @davidtwco
|
Btw, is this an opsem guarantee of some sort? Does it also affect Rust-to-Rust calls and hence needs an implementation in Miri? |
|
I'm not sure when something qualifies as an opsem guarantee. The padding being cleared is something that users should be able to rely on, and it will be part of the reference. That the bytes are zeroed specifically is an implementation detail (idk we could pick The clearing is only important though when the secure boundary is actually crossed, and that is impossible to do within a single program. So practically an implementation in Miri doesn't have much use, I don't think there is any new UB that should be caught. I may be missing something though. |
|
Thinking about it again, I think the change you are making here is impossible to observe in a UB-free program. Padding is reset to uninit when an argument is passed, so any program trying to check if there's a 0 there or not has UB. |
View all comments
tracking issue: #81391
tracking issue: #75835
RFC: rust-lang/rfcs#3884
related: #147697
quick context: cmse creates a distinction between code running in secure mode and non-secure mode (think kernel space versus user space). Secure mode has access to data (e.g. encryption keys) that must not leak to non-secure mode. They use a special calling convention that clears unused registers, but padding in arguments/return values can contain stale secure data.
This PR clears the padding bytes (and similar, e.g. space not used in any variant of a union/enum) when values are passed over the secure boundary.
Separately we'll have a lint to warn on enums and unions being passed across the boundary: for them we can't statically know whether the variant that is passed contains padding.
This is conceptually modeled after a similar feature in
clang(implementation).cc @Jules-Bertholet
r? @davidtwco