Repository navigation
fix(sandbox): zero-initialize sigset_t before assume_init - #5194
Conversation
|
/ok to test 3eeab41 |
|
Not too familiar with section of the code but anyways, AI comment The new tests verify signal behavior, but don’t detect the original uninitialized-memory bug. Please describe them as behavior checks rather than Miri-equivalent checks. just verify we are not introducing any regression with this |
|
Fair point, reworded: the tests are behavior checks for the mask operations (block, clear, restore), not a detector for the uninitialized read itself. Miri can't run this crate (FFI, seccomp, tokio), that's all. No regressions that I can find: everything is green since your /ok to test, rust tests on all three triples included, plus the podman CI e2e I ran locally before opening. The only red is the advisory compatibility review, which failed with its own RuntimeError before producing an assessment, so there's nothing to act on there. |
mrunalp
added
area:sandbox
|
Label |
|
/ok to test 3eeab41 |
musl's sigfillset and sigemptyset write only the first word of the 128-byte sigset_t on x86_64 and aarch64, so assuming the MaybeUninit value is initialized reads 120 uninitialized bytes in block_launcher_signals and install_child_signal_mask. Start both from MaybeUninit::zeroed() so the whole value is defined regardless of how much the libc writes, and correct the SAFETY comments accordingly. An all-zero sigset_t is a valid empty set, so the mask handed to the kernel is unchanged. The nix SigSet constructors were rejected as an alternative because they hold the same uninit + assume_init pattern. Adds tests that call both functions on their own thread, assert the resulting mask state, and restore the previous mask afterwards, since Miri cannot run this crate's FFI and seccomp-heavy tests. Closes NVIDIA#5187 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
Head branch was pushed to by a user without write access
3eeab41 to
b07594e
Compare
|
/ok to test b07594e |
Summary
block_launcher_signalsandinstall_child_signal_masknow build theirlibc::sigset_tfromMaybeUninit::zeroed()instead ofuninit(). On 64-bit musl,sigfillsetandsigemptysetwrite only the first word of the 128-bytesigset_t, soassume_init()was reading uninitialized bytes. The mask handed to the kernel is unchanged; this removes the undefined behavior and fixes theSAFETYcomments that claimed the libc calls fully initialized the value.Related Issue
Closes #5187
Part of #5179
Note for maintainers: #5187 still needs
state:acceptedfor the PR Issue Acceptance check to pass.Changes
block_launcher_signalsandinstall_child_signal_maskuseMaybeUninit::zeroed()// SAFETY:comments on bothassume_init()callsSigSet::all()/empty(): nix's own constructors hold the sameuninit+assume_initpattern this PR removesRelease Notes
Testing
cargo fmt --check -p openshell-sandboxcargo clippy -p openshell-sandbox --all-targets -- -D warningscargo test -p openshell-sandbox(new tests pass; one lib test flakes under full parallelism on this host and also fails on clean main)mise run e2e:podman:ci(sandbox image built from this tree, musl target)zeroed()initialization fixes. Miri cannot run this crate (FFI, seccomp, tokio).Checklist