Skip to content

cmse: lint on unions crossing the secure boundary - #147697

Open
folkertdev wants to merge 2 commits into
rust-lang:mainfrom
folkertdev:cmse-lint-on-uninitialized
Open

cmse: lint on unions crossing the secure boundary#147697
folkertdev wants to merge 2 commits into
rust-lang:mainfrom
folkertdev:cmse-lint-on-uninitialized

Conversation

@folkertdev

@folkertdev folkertdev commented Oct 14, 2025

Copy link
Copy Markdown
Contributor

View all comments

tracking issue: #81391
tracking issue: #75835

Adds a cmse_uninitialized_leak lint.

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

@folkertdev folkertdev added F-cmse_nonsecure_entry `#![feature(cmse_nonsecure_entry)]` F-abi_cmse_nonsecure_call `#![feature(abi_cmse_nonsecure_call)]` labels Oct 14, 2025
@rustbot rustbot added A-test-infra-minicore Area: `minicore` test auxiliary and `//@ add-core-stubs` S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 14, 2025
@rust-log-analyzer

This comment has been minimized.

@folkertdev
folkertdev force-pushed the cmse-lint-on-uninitialized branch from a344d30 to 514010e Compare October 15, 2025 09:09
@rust-log-analyzer

This comment has been minimized.

@folkertdev
folkertdev force-pushed the cmse-lint-on-uninitialized branch from 514010e to 9d276b5 Compare October 15, 2025 11:03
@rust-log-analyzer

This comment has been minimized.

@folkertdev
folkertdev force-pushed the cmse-lint-on-uninitialized branch from 9d276b5 to 4a269f5 Compare October 15, 2025 11:55
@rust-log-analyzer

This comment has been minimized.

@folkertdev
folkertdev force-pushed the cmse-lint-on-uninitialized branch from 4a269f5 to 0b424f6 Compare October 15, 2025 16:49
@folkertdev

Copy link
Copy Markdown
Contributor Author

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)

@folkertdev
folkertdev marked this pull request as ready for review October 15, 2025 19:29
@rustbot

rustbot commented Oct 15, 2025

Copy link
Copy Markdown
Collaborator

HIR ty lowering was modified

cc @fmease

This PR modifies tests/auxiliary/minicore.rs.

cc @jieyouxu

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 15, 2025

@davidtwco davidtwco left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Implementation LGTM, two nits, but will need t-lang approval for a new lint

View changes since this review

Comment thread tests/ui/cmse-nonsecure/cmse-nonsecure-entry/params-via-stack.rs Outdated
use minicore::*;

#[repr(Rust)]
pub union ReprRustUnionU64 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add cases where the unions are contained within other types to these tests?

@davidtwco davidtwco added the I-lang-nominated Nominated for discussion during a lang team meeting. label Oct 16, 2025
@folkertdev
folkertdev force-pushed the cmse-lint-on-uninitialized branch from 0b424f6 to 4586300 Compare October 16, 2025 20:05

@folkertdev folkertdev left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

View changes since this review

Comment on lines +49 to +58
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 | | }
| |_____^
|

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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).

@traviscross traviscross added the P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang label Oct 22, 2025
@joshtriplett

Copy link
Copy Markdown
Member

Ideally we'd warn on any type that may have uninitialized parts, but I haven't figured out a good way to do that yet.

Would that also include padding?

In any case, this seems wildly useful for many users, not just cmse. The Linux kernel would likely benefit from this, for instance, to avoid leaking uninitialized bits to userspace.

On that basis I'm wondering if we should aspirationally call this uninitialized_leak or similar, without cmse in it.

Is there some means by which we could allow the user to suppress this not by allow but by demonstrating that they've properly zero-initialized it? Or is the point that LLVM doesn't guarantee that because it might un-zero that memory (e.g. copying without caring what value ends up in the padding / unused-union-bits / etc)?

@folkertdev

Copy link
Copy Markdown
Contributor Author

Would that also include padding?

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.

Is there some means by which we could allow the user to suppress this not by allow but by demonstrating that they've properly zero-initialized it? Or is the point that LLVM doesn't guarantee that because it might un-zero that memory (e.g. copying without caring what value ends up in the padding / unused-union-bits / etc)?

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?

@joshtriplett

Copy link
Copy Markdown
Member

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?

@folkertdev

Copy link
Copy Markdown
Contributor Author

What I had in mind is to use MaybeUninit::zeroed, set the relevant fields, and assume_init.

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).

@traviscross traviscross removed I-lang-nominated Nominated for discussion during a lang team meeting. P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang labels Oct 29, 2025
@rust-bors

This comment has been minimized.

@folkertdev
folkertdev force-pushed the cmse-lint-on-uninitialized branch from 3095f88 to 1c72768 Compare July 1, 2026 15:36
@rustbot

This comment has been minimized.

Comment thread compiler/rustc_lint/src/cmse_uninitialized_leak.rs Outdated
@folkertdev
folkertdev force-pushed the cmse-lint-on-uninitialized branch from 1c72768 to 86edf48 Compare July 2, 2026 13:17
@rust-log-analyzer

This comment has been minimized.

jhpratt added a commit to jhpratt/rust that referenced this pull request Jul 23, 2026
…-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
jhpratt added a commit to jhpratt/rust that referenced this pull request Jul 23, 2026
…-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
jhpratt added a commit to jhpratt/rust that referenced this pull request Jul 23, 2026
…-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
jhpratt added a commit to jhpratt/rust that referenced this pull request Jul 23, 2026
…-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
rust-timer added a commit that referenced this pull request Jul 24, 2026
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
@rust-bors

This comment has been minimized.

@folkertdev
folkertdev force-pushed the cmse-lint-on-uninitialized branch from 86edf48 to 7c4e28d Compare July 24, 2026 10:08
@rustbot

rustbot commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

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.

@folkertdev
folkertdev force-pushed the cmse-lint-on-uninitialized branch 4 times, most recently from 96e8746 to a875bac Compare July 24, 2026 19:19
@folkertdev

Copy link
Copy Markdown
Contributor Author

@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.

@folkertdev folkertdev added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jul 24, 2026
@folkertdev
folkertdev force-pushed the cmse-lint-on-uninitialized branch from a875bac to 7a06e95 Compare July 24, 2026 19:24

@davidtwco davidtwco left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@folkertdev folkertdev added the I-lang-nominated Nominated for discussion during a lang team meeting. label Aug 4, 2026
@folkertdev

Copy link
Copy Markdown
Contributor Author

Nominating this for T-lang.

Something I'd like input on is the name, cmse_uninitialized_leak, and what it means.

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.

@Jules-Bertholet

Copy link
Copy Markdown
Contributor

Because this is a lint, this PR will need T-lang FCP.

You can probably bundle this into the broader CMSE FCP?

@traviscross traviscross added the P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang label Aug 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-test-infra-minicore Area: `minicore` test auxiliary and `//@ add-core-stubs` F-abi_cmse_nonsecure_call `#![feature(abi_cmse_nonsecure_call)]` F-cmse_nonsecure_entry `#![feature(cmse_nonsecure_entry)]` I-lang-nominated Nominated for discussion during a lang team meeting. I-lang-radar Items that are on lang's radar and will need eventual work or consideration. needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants