cmse: lint on unions crossing the secure boundary - #147697
Conversation
This comment has been minimized.
This comment has been minimized.
a344d30 to
514010e
Compare
This comment has been minimized.
This comment has been minimized.
514010e to
9d276b5
Compare
This comment has been minimized.
This comment has been minimized.
9d276b5 to
4a269f5
Compare
This comment has been minimized.
This comment has been minimized.
4a269f5 to
0b424f6
Compare
|
r? @davidtwco This seems useful just for parity with clang. The code is built to be extended to cover more cases of types possibly containing uninitialized memory, but by the looks of things there isn't currently a straightforward way to detect such types (cc #t-compiler/help > check whether a type can be (partially) uninitialized) |
| use minicore::*; | ||
|
|
||
| #[repr(Rust)] | ||
| pub union ReprRustUnionU64 { |
There was a problem hiding this comment.
Can you add cases where the unions are contained within other types to these tests?
0b424f6 to
4586300
Compare
There was a problem hiding this comment.
The lint will only fire when the cmse ABIs are enabled, but I suppose the name does sort of "leak".
the OP here provides some context. The bigger picture is in this draft RFC that I plan to formally submit soon.
| warning: passing a union across the security boundary may leak information | ||
| --> $DIR/return-uninitialized.rs:46:5 | ||
| | | ||
| LL | / match 0 { | ||
| LL | | | ||
| LL | | 0 => Wrapper(ReprRustUnionU64 { _unused: 1 }), | ||
| LL | | _ => Wrapper(ReprRustUnionU64 { _unused: 2 }), | ||
| LL | | } | ||
| | |_____^ | ||
| | |
There was a problem hiding this comment.
would it make sense to warn in the individual arms of the match instead? I think as a user that would be better in this simple case, though I don't know that we can make that robust (e.g. thinking about labeled blocks).
Would that also include padding? In any case, this seems wildly useful for many users, not just On that basis I'm wondering if we should aspirationally call this Is there some means by which we could allow the user to suppress this not by |
Ideally, yes. But I don't have a good strategy for actually achieving that. Based on #t-compiler/help > check whether a type can be (partially) uninitialized @ 💬 maybe there are parts of safe transmute that are helpful here.
I'm not familiar enough with the opsem details here, but in any case I don't think we'd want to tie our lints to LLVM implementation details that much? |
|
I'm not proposing to depend on LLVM details. I was asking how we can handle code that's doing the correct thing (whatever that might be), such as ensuring the uninitialized memory is zero. In C, you would ensure the union is zero-initialized, then initialize the correct field, then return it. What's the equivalent operation that you can do in Rust, and can we ensure that we don't emit the lint if you properly do that? |
|
What I had in mind is to use At least in that case, I don't think we have a good way of checking whether the value is properly initialized without actually evaluating the program (e.g. with miri). |
This comment has been minimized.
This comment has been minimized.
3095f88 to
1c72768
Compare
This comment has been minimized.
This comment has been minimized.
1c72768 to
86edf48
Compare
This comment has been minimized.
This comment has been minimized.
…-padding, r=davidtwco cmse: clear variant-dependent padding in `enum`s tracking issue: rust-lang#81391 tracking issue: rust-lang#75835 Since rust-lang#157397 we clear variant-independent padding, bytes that are padding for all valid values of the type. This PR extends that idea to also clear variant-dependent padding, where we have to check what variant a (nested) enum has to determine which bytes are padding so we can clear them. With these changes the lint from rust-lang#147697 can lint on just `union`s. r? @davidtwco cc @RalfJung @Jules-Bertholet
…-padding, r=davidtwco cmse: clear variant-dependent padding in `enum`s tracking issue: rust-lang#81391 tracking issue: rust-lang#75835 Since rust-lang#157397 we clear variant-independent padding, bytes that are padding for all valid values of the type. This PR extends that idea to also clear variant-dependent padding, where we have to check what variant a (nested) enum has to determine which bytes are padding so we can clear them. With these changes the lint from rust-lang#147697 can lint on just `union`s. r? @davidtwco cc @RalfJung @Jules-Bertholet
…-padding, r=davidtwco cmse: clear variant-dependent padding in `enum`s tracking issue: rust-lang#81391 tracking issue: rust-lang#75835 Since rust-lang#157397 we clear variant-independent padding, bytes that are padding for all valid values of the type. This PR extends that idea to also clear variant-dependent padding, where we have to check what variant a (nested) enum has to determine which bytes are padding so we can clear them. With these changes the lint from rust-lang#147697 can lint on just `union`s. r? @davidtwco cc @RalfJung @Jules-Bertholet
…-padding, r=davidtwco cmse: clear variant-dependent padding in `enum`s tracking issue: rust-lang#81391 tracking issue: rust-lang#75835 Since rust-lang#157397 we clear variant-independent padding, bytes that are padding for all valid values of the type. This PR extends that idea to also clear variant-dependent padding, where we have to check what variant a (nested) enum has to determine which bytes are padding so we can clear them. With these changes the lint from rust-lang#147697 can lint on just `union`s. r? @davidtwco cc @RalfJung @Jules-Bertholet
Rollup merge of #159466 - folkertdev:clear-variant-dependent-padding, r=davidtwco cmse: clear variant-dependent padding in `enum`s tracking issue: #81391 tracking issue: #75835 Since #157397 we clear variant-independent padding, bytes that are padding for all valid values of the type. This PR extends that idea to also clear variant-dependent padding, where we have to check what variant a (nested) enum has to determine which bytes are padding so we can clear them. With these changes the lint from #147697 can lint on just `union`s. r? @davidtwco cc @RalfJung @Jules-Bertholet
This comment has been minimized.
This comment has been minimized.
86edf48 to
7c4e28d
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. |
96e8746 to
a875bac
Compare
|
@rustboy ready @davidtwco can you give this another pass before I nominate it for T-lang. We discussed this lint during a recent design meeting (#t-lang/meetings > Design meeting 2026-07-22) so overall T-lang is on board, but I think lints need FCP regardless. |
a875bac to
7a06e95
Compare
|
Nominating this for T-lang. Something I'd like input on is the name, It contains "cmse" because it triggers only on the cmse calling conventions. It is possible we want to generalize the behavior, so maybe the name should be more general. The name does not contain "union". That is partially historical (earlier iterations of this lint also linted on enums), but also the lint is currently named after the concept that it is about, not the specifics of how it performs the check. Because this is a lint, this PR will need T-lang FCP. |
You can probably bundle this into the broader CMSE FCP? |
View all comments
tracking issue: #81391
tracking issue: #75835
Adds a
cmse_uninitialized_leaklint.When a union passes from secure to non-secure (so, passed as an argument to a non-secure call, or returned by a non-secure entry), warn that there may be secure information lingering in the unused or uninitialized parts of a union value.
This lint matches the behavior of clang (see https://godbolt.org/z/vq9xnrnEs). Like clang we warn at the use site, so that individual uses could be annotated with
#[allow(cmse_uninitialized_leak)].It is still unclear whether a union value where all fields are equally large and allow the same bit patterns can be considered initialized (see rust-lang/unsafe-code-guidelines#438), so for now we just warn on any
union.r? @ghost