patina_internal_collections: Fix UB memory read in Node fields by garybeihl · Pull Request #1152 · OpenDevicePartnership/patina

garybeihl · 2025-12-03T16:38:29Z

Description

Fixes an undefined behavior issue where Cell::set() reads uninitialized memory during linked list creation in Storage::resize().

Root Cause

Cell::set() internally uses mem::replace(), which reads the old value before writing the new one.
When Storage::resize() allocates new nodes and calls build_linked_list(), the Cell fields contain uninitialized memory.
Reading uninitialized memory is undefined behavior, even if immediately overwritten. Unwanted compiler "optimizations" could follow.

Impact

Any package using patina_internal_collections.
Potential memory corruption and non-deterministic errors
Detected by Miri testing (issue Consider running unit tests (or a subset of them) under Miri #560)

Fix

Initialize all Cell fields using ptr::write() before build_linked_list()
Use addr_of_mut!() to read field pointers without creating references to uninitialized data

Introduced by

PR Replace AtomicPtr with Cell in patina_internal_collections. #1050 (Nov 13, 2025) Replace AtomicPtr with Cell in patina_internal_collections
AtomicPtr::store() writes without reading the old value, but Cell::set() uses mem::replace() which reads before writing

Related to #560

How This Was Tested

Tested with: cargo +nightly-2025-09-19 miri test -p patina_dxe_core.
7 tests now pass (previously 0/469 due to this UB issue).

Integration Instructions

N/A

codecov · 2025-12-03T16:40:55Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

core/patina_internal_collections/src/node.rs

Javagedes · 2025-12-19T16:38:21Z

Hi @garybeihl , I noticed you've been keeping this PR up to date, but not working on it (which is completely fine).

I just wanted to make sure you were not waiting on additional information from any of us that have commented on this PR (i.e. make sure we are not blocking you in any way)!

garybeihl · 2025-12-19T16:59:42Z

No - not blocked - I uploaded a version of the changes that uses MaybeUninit and D: Default - just waiting for further comments or change requests. If you could have a look when you get a chance, that would be great - thanks!

core/patina_internal_collections/src/node.rs

…ment Replace Default trait requirement with MaybeUninit wrapper for Node data field. Only initialize data when nodes move from available to in-use list. Fixes UB where Cell::set() was reading uninitialized memory.

Add safety documentation to all unsafe blocks in production code. Add allow(clippy::undocumented_unsafe_blocks) to test modules to avoid redundant safety comments for test assertions.

garybeihl · 2026-02-12T20:08:44Z

Is there anything more for me to do here? If not, can I get another approval?

makubacki

@Javagedes & @joschock should have a final review as well.

patina-automation · 2026-03-05T19:20:58Z

✅ QEMU Validation Passed

All QEMU validation jobs completed successfully.

Note: Windows Q35 is only built (QEMU boot is disabled due to QEMU vfat issue).

Workflow run: https://github.com/OpenDevicePartnership/patina/actions/runs/22733280504

Boot Time to EFI Shell

Platform	Elapsed
Linux Q35	34.9s
Linux SBSA	41.9s

This comment was automatically generated by the Patina QEMU PR Validation Post workflow.

garybeihl · 2026-03-05T19:37:36Z

Ping - please review so I can close this and move on to other Miri-found issues. Thanks!

makubacki

Approving latest

makubacki · 2026-03-05T21:25:39Z

core/patina_internal_collections/src/node.rs

        let buffer = unsafe {
            slice::from_raw_parts_mut::<'a, Node<D>>(
                slice as *mut [u8] as *mut Node<D>,
                slice.len() / mem::size_of::<Node<D>>(),
            )
        };


Does this need to go through MaybeUnit<Node<D>> too?

Good catch — yes, expand() has the same problem. It creates a &mut [Node<D>] from raw bytes without going through MaybeUninit, which is UB for the same reason. I'll update it to use MaybeUninit<Node<D>> as well.

makubacki · 2026-03-05T21:29:20Z

core/patina_internal_collections/src/node.rs

    D: SliceKey,
 {
-    pub data: D,
+    pub data: MaybeUninit<D>,


Does this still need to be pub with functions like pub unsafe fn data() and pub unsafe fn data_mut() having been added? Or could it at least be pub(crate)?

You're right — nothing outside the crate accesses the field directly; all external access goes through the data() and data_mut() accessors. I'll change it to pub(crate).

makubacki · 2026-03-05T21:32:31Z

core/patina_internal_collections/src/node.rs

@@ -216,7 +229,13 @@
        for i in 0..self.capacity() {


Is for i in 0..self.capacity() supposed to be for i in 0..self.len()? That's what the safety comment seems to indicate below.

makubacki · 2026-03-05T21:34:30Z

core/patina_internal_collections/src/node.rs


 #[cfg(test)]
 #[coverage(off)]
+#[allow(clippy::undocumented_unsafe_blocks)]


We're generally trying to provide safety comments more granularly (even for tests) to avoid needed to apply this at the module level.

github-actions bot added the impact:security Has a security impact label Dec 3, 2025

garybeihl added the type:bug Something isn't working label Dec 3, 2025

garybeihl force-pushed the fix-node-uninit branch 2 times, most recently from 492ef76 to 7a57e63 Compare December 3, 2025 16:57

os-d requested review from Javagedes, joschock, makubacki and os-d December 3, 2025 17:07

os-d reviewed Dec 3, 2025

View reviewed changes

core/patina_internal_collections/src/node.rs Outdated Show resolved Hide resolved

joschock reviewed Dec 3, 2025

View reviewed changes

core/patina_internal_collections/src/node.rs Outdated Show resolved Hide resolved

core/patina_internal_collections/src/node.rs Outdated Show resolved Hide resolved

Javagedes reviewed Dec 3, 2025

View reviewed changes

core/patina_internal_collections/src/node.rs Outdated Show resolved Hide resolved

Javagedes reviewed Dec 3, 2025

View reviewed changes

core/patina_internal_collections/src/node.rs Outdated Show resolved Hide resolved

garybeihl force-pushed the fix-node-uninit branch 4 times, most recently from f0178c1 to b1385f8 Compare December 9, 2025 00:26

garybeihl force-pushed the fix-node-uninit branch 2 times, most recently from ae5dcec to 7fcea9b Compare December 16, 2025 00:41

joschock approved these changes Dec 19, 2025

View reviewed changes

garybeihl requested review from Javagedes, joschock and os-d December 19, 2025 18:30

garybeihl force-pushed the fix-node-uninit branch 3 times, most recently from a3c3321 to 490f0cf Compare December 23, 2025 21:23

garybeihl force-pushed the fix-node-uninit branch from 490f0cf to 82593c8 Compare January 7, 2026 16:29

makubacki reviewed Jan 12, 2026

View reviewed changes

core/patina_internal_collections/src/node.rs Outdated Show resolved Hide resolved

core/patina_internal_collections/src/node.rs Show resolved Hide resolved

core/patina_internal_collections/src/node.rs Outdated Show resolved Hide resolved

garybeihl force-pushed the fix-node-uninit branch 4 times, most recently from f6e8da6 to 84636f4 Compare January 15, 2026 15:04

garybeihl force-pushed the fix-node-uninit branch from 84636f4 to e2a9857 Compare January 15, 2026 15:28

patina_internal_collections: Add safety comments to fix clippy warnings

f669b07

Add safety documentation to all unsafe blocks in production code. Add allow(clippy::undocumented_unsafe_blocks) to test modules to avoid redundant safety comments for test assertions.

makubacki force-pushed the main branch from a62f178 to c7eaf05 Compare February 3, 2026 17:25

Merge branch 'main' into fix-node-uninit

ae44053

makubacki approved these changes Feb 12, 2026

View reviewed changes

Merge remote-tracking branch 'upstream/main' into fix-node-uninit

17c85c6

makubacki reviewed Mar 5, 2026

View reviewed changes

Conversation

garybeihl commented Dec 3, 2025 • edited by makubacki Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Root Cause

Impact

Fix

Introduced by

How This Was Tested

Integration Instructions

Uh oh!

codecov bot commented Dec 3, 2025

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Javagedes commented Dec 19, 2025

Uh oh!

garybeihl commented Dec 19, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

garybeihl commented Feb 12, 2026

Uh oh!

makubacki left a comment

Choose a reason for hiding this comment

Uh oh!

patina-automation bot commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ QEMU Validation Passed

Boot Time to EFI Shell

Uh oh!

garybeihl commented Mar 5, 2026

Uh oh!

makubacki left a comment

Choose a reason for hiding this comment

Uh oh!

makubacki Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

garybeihl Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

makubacki Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

garybeihl Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

makubacki Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

makubacki Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

garybeihl commented Dec 3, 2025 •

edited by makubacki

Loading

patina-automation bot commented Mar 5, 2026 •

edited

Loading