Skip to content

Soundness bug in DstLayout::extend: #[repr(C, packed(P))] wrapping a dynamically-padded slice DST drops inner alignment, causing UB in safe Rust #3900

Closed as not planned

Description

@joshlf

EDIT: Auto-discovered by an agent on an old checkout of zerocopy

Agent-discovered bug, and agent-authored writeup.

Summary

When a #[repr(C, packed)] or #[repr(C, packed(P))] struct Outer has a trailing field field (Inner) that is a slice DST (SizeInfo::SliceDst(TrailingSliceLayout { offset: trailing_offset, elem_size })) with field.align > P and field.requires_dynamic_padding(), DstLayout::extend (zerocopy/src/layout.rs:341-459) clamps the outer struct's alignment to max(self.align, min(field.align, P)) <= P < field.align, adds offset + trailing_offset, and discards field.align.

However, in Rust, #[repr(packed(P))] on Outer only reduces the alignment of Outer's direct fields for the purpose of positioning them inside Outer—it does not strip internal trailing padding from Inner. At runtime, Inner's size for elems elements is still rounded up to a multiple of field.align before Outer rounds the composite size up to outer_align:

$$\operatorname{size_of_val}(\text{Outer}, \text{elems}) = \operatorname{align_to}!\left(\text{offset} + \operatorname{align_to}(\text{trailing_offset} + \text{elem_size} \times \text{elems},; \text{field.align}),; \text{outer_align}\right)$$

Whereas Outer::LAYOUT (DstLayout { align: outer_align, size_info: SliceDst { offset: offset + trailing_offset, elem_size } }) computes:

$$\text{Outer::size_for_metadata}(\text{elems}) = \operatorname{align_to}(\text{offset} + \text{trailing_offset} + \text{elem_size} \times \text{elems},; \text{outer_align})$$

Because outer_align <= P < field.align, Outer::LAYOUT underestimates the size of Outer whenever (trailing_offset + elem_size * elems) % field.align != 0, violating KnownLayout::LAYOUT's safety invariant (zerocopy/src/lib.rs:781-795) and causing Undefined Behavior in 100% safe Rust via FromBytes::ref_from_bytes (constructing an out-of-bounds reference &Outer) and FromZeros::new_box_zeroed_with_elems (heap under-allocation and dangling Box<Outer> / mismatched alloc/dealloc layout).

Affected Files

  • zerocopy/src/layout.rs (DstLayout::extend, prove_dst_layout_extend)
  • zerocopy/zerocopy-derive/src/derive/known_layout.rs
  • zerocopy/src/lib.rs (KnownLayout, FromBytes::ref_from_bytes, FromZeros::new_box_zeroed_with_elems)
  • zerocopy/src/util/mod.rs (new_box)

Detailed Execution Flow

Root Cause in DstLayout::extend

In zerocopy/src/layout.rs:373-448:

let field_align = min(field.align, max_align);

// The struct's alignment is the maximum of its previous alignment and
// `field_align`.
let align = max(self.align, field_align);
...
SizeInfo::SliceDst(TrailingSliceLayout {
    offset: trailing_offset,
    elem_size,
}) => {
    let offset = match offset.checked_add(trailing_offset) {
        Some(offset) => offset,
        None => const_panic!("`field` cannot be appended without the total size overflowing `usize`"),
    };
    SizeInfo::SliceDst(TrailingSliceLayout { offset, elem_size })
}

When field is a Sized type (SizeInfo::Sized { size: field_size }), field_size has already been rounded up to a multiple of field.align by Inner's own DstLayout::pad_to_align (zerocopy/src/layout.rs:466-491). Thus, clamping field_align to min(field.align, max_align) when extending Outer preserves Inner's internal trailing padding.

By contrast, when field is a SliceDst, Inner's DstLayout::pad_to_align was a no-op because Inner's trailing padding is deferred to runtime using Inner::LAYOUT.align (field.align). When Outer has #[repr(C, packed(P))] with P < field.align, DstLayout::extend replaces field.align with align <= P < field.align, so Inner's trailing padding is never accounted for at compile time or at runtime.

Two Sub-cases Where Outer::LAYOUT Computes the Wrong Size

Sub-case 1: Dynamic internal trailing padding (elem_size % field.align != 0)

#[derive(KnownLayout, FromBytes, Immutable)]
#[repr(C)]
struct Inner {
    a: u32,
    b: [u16],
} // Inner::LAYOUT: align = 4, offset = 4, elem_size = 2

#[derive(KnownLayout, FromBytes, Immutable)]
#[repr(C, packed)]
struct Outer {
    p: u8,
    inner: Inner,
} // Outer::LAYOUT: align = 1, offset = 5, elem_size = 2

For elems = 1:

  1. Outer::LAYOUT computes Outer::size_for_metadata(1) = align_to(5 + 2 * 1, 1) = 7 bytes (or 6 bytes without p: u8).
  2. Rust's core::mem::size_of_val::<Outer> is 1 + align_to(4 + 2 * 1, 4) = 1 + 8 = 9 bytes (or 8 bytes without p: u8).

Sub-case 2: Constant non-zero internal trailing padding after the slice (trailing_offset % field.align != 0, even when elem_size % field.align == 0)

#[derive(KnownLayout, FromBytes, Immutable)]
#[repr(C)]
struct Inner2 {
    a: u32,
    b: u8,
    c: [[u8; 4]],
} // Inner2::LAYOUT: align = 4, offset = 5, elem_size = 4 (always has 3 bytes of trailing padding)

#[derive(KnownLayout, FromBytes, Immutable)]
#[repr(C, packed)]
struct Outer2 {
    inner: Inner2,
} // Outer2::LAYOUT: align = 1, offset = 5, elem_size = 4

For elems = 0:

  1. Outer2::LAYOUT computes Outer2::size_for_metadata(0) = align_to(5 + 4 * 0, 1) = 5 bytes.
  2. Rust's core::mem::size_of_val::<Outer2> is align_to(5 + 4 * 0, 4) = 8 bytes.

Safe-Rust Undefined Behavior Exploits

  1. Out-of-bounds reference creation via FromBytes::ref_from_bytes:
    • Calling Outer::ref_from_bytes(&[0u8; 7]).unwrap() validates bytes_len == 7 against Outer::LAYOUT (offset = 5, elem_size = 2, align = 1), succeeds with metadata elems = 1, and returns &Outer pointing to a 7-byte stack allocation.
    • However, core::mem::size_of_val(outer) == 9. Constructing a reference &Outer whose size_of_val (9 bytes) exceeds the underlying allocation (7 bytes) is immediate Undefined Behavior in Rust/Miri (pointer not dereferenceable: pointer must be dereferenceable for 9 bytes, but got alloc which is only 7 bytes from the end of the allocation).
    • Similarly, Outer2::ref_from_bytes(&[0u8; 5]).unwrap() succeeds with metadata elems = 0 and constructs a &Outer2 of size_of_val == 8 over a 5-byte allocation.
  2. Heap buffer under-allocation, dangling Box, and mismatched alloc/dealloc layout via FromZeros::new_box_zeroed_with_elems:
    • Calling Outer::new_box_zeroed_with_elems(1).unwrap() calls util::new_box::<Outer>(1, alloc_zeroed) (zerocopy/src/util/mod.rs:386-468).
    • Outer::size_for_metadata(1) returns Some(7), so new_box allocates 7 bytes with Layout::from_size_align_unchecked(7, 1) and constructs Box::<Outer>::from_raw with slice metadata 1.
    • Since size_of_val::<Outer> for metadata 1 is 9 bytes, Box::from_raw immediately triggers UB in Miri (constructing invalid value of type std::boxed::Box<Outer>: encountered a dangling box (going beyond the bounds of its allocation)), and dropping the Box<Outer> passes Layout::for_value(&*b) (size = 9, align = 1) to dealloc on a 7-byte allocation.

Why Kani Missed This in prove_dst_layout_extend

In zerocopy/src/layout.rs:2135-2167, prove_dst_layout_extend modeled the SizeInfo::SliceDst branch as:

let field_analog =
    Layout::from_size_align(field_offset, field_align.get()).unwrap();

if let Ok((actual_composite, actual_offset)) = base_analog.extend(field_analog)
{
    assert_eq!(actual_offset, offset);
    assert_eq!(actual_composite.size(), composite_offset);
    assert_eq!(actual_composite.align(), composite.align.get());
}

This treated the SliceDst field as if it were a Sized field of size field_offset and already-clamped alignment field_align, without modeling either:

  1. field's own .pad_to_align() to field.align (before Outer's extension), or
  2. Non-zero slice element counts elems * field_elem_size.

Proof of Concept & Attachments

poc.rs
//! PoC: `DstLayout::extend` drops inner alignment when `#[repr(C, packed(P))]`
//! wraps a slice DST with internal trailing padding (`field.requires_dynamic_padding()`),
//! causing `Outer::LAYOUT` to underestimate the size of `Outer` and leading to
//! Undefined Behavior in 100% safe Rust.

#![forbid(unsafe_code)]

use zerocopy::{FromBytes, FromZeros, Immutable, KnownLayout};

// Sub-case 1: `elem_size % field.align != 0` (dynamic internal trailing padding)
// `Inner` has `align = 4, offset = 4, elem_size = 2`.
#[derive(KnownLayout, FromBytes, Immutable)]
#[repr(C)]
struct Inner {
    a: u32,
    b: [u16],
}

// `Outer` has `#[repr(C, packed)]`, so `Outer::LAYOUT` has `align = 1, offset = 5, elem_size = 2`.
// For `elems = 1`:
// - `Outer::LAYOUT` computes size = `5 + 2 * 1 = 7` bytes.
// - `core::mem::size_of_val::<Outer>` is `1 + align_to(4 + 2 * 1, 4) = 9` bytes.
#[derive(KnownLayout, FromBytes, Immutable)]
#[repr(C, packed)]
struct Outer {
    p: u8,
    inner: Inner,
}

// Sub-case 2: `trailing_offset % field.align != 0` with `elem_size % field.align == 0`
// (constant non-zero internal trailing padding after the dynamic slice)
// `Inner2` has `align = 4, offset = 5, elem_size = 4`.
#[derive(KnownLayout, FromBytes, Immutable)]
#[repr(C)]
struct Inner2 {
    a: u32,
    b: u8,
    c: [[u8; 4]],
}

// `Outer2` has `#[repr(C, packed)]`, so `Outer2::LAYOUT` has `align = 1, offset = 5, elem_size = 4`.
// For `elems = 0`:
// - `Outer2::LAYOUT` computes size = `5 + 4 * 0 = 5` bytes.
// - `core::mem::size_of_val::<Outer2>` is `align_to(5 + 4 * 0, 4) = 8` bytes.
#[derive(KnownLayout, FromBytes, Immutable)]
#[repr(C, packed)]
struct Outer2 {
    inner: Inner2,
}

#[test]
fn poc_layout_size_mismatch() {
    // Check `KnownLayout` vs `core::mem::size_of_val` on a sufficiently large buffer first:
    let large_buf = [0u8; 16];
    let (outer, rest) = Outer::ref_from_prefix_with_elems(&large_buf, 1).unwrap();
    let zerocopy_size = large_buf.len() - rest.len();
    let rustc_size = core::mem::size_of_val(outer);
    eprintln!(
        "Sub-case 1 (elems = 1): zerocopy size = {zerocopy_size}, core::mem::size_of_val = {rustc_size}"
    );
    assert_eq!(zerocopy_size, 7);
    assert_eq!(rustc_size, 9);

    let (outer2, rest2) = Outer2::ref_from_prefix_with_elems(&large_buf, 0).unwrap();
    let zerocopy_size2 = large_buf.len() - rest2.len();
    let rustc_size2 = core::mem::size_of_val(outer2);
    eprintln!(
        "Sub-case 2 (elems = 0): zerocopy size = {zerocopy_size2}, core::mem::size_of_val = {rustc_size2}"
    );
    assert_eq!(zerocopy_size2, 5);
    assert_eq!(rustc_size2, 8);
}

#[test]
fn poc_ref_from_bytes_oob_sub_case_1() {
    // 7-byte buffer: `Outer::ref_from_bytes` succeeds with `elems = 1`,
    // creating `&Outer` of `size_of_val == 9` pointing to a 7-byte allocation!
    let buf = [0u8; 7];
    let outer = Outer::ref_from_bytes(&buf).unwrap();
    assert_eq!(core::mem::size_of_val(outer), 9);
}

#[test]
fn poc_ref_from_bytes_oob_sub_case_2() {
    // 5-byte buffer: `Outer2::ref_from_bytes` succeeds with `elems = 0`,
    // creating `&Outer2` of `size_of_val == 8` pointing to a 5-byte allocation!
    let buf = [0u8; 5];
    let outer2 = Outer2::ref_from_bytes(&buf).unwrap();
    assert_eq!(core::mem::size_of_val(outer2), 8);
}

#[test]
fn poc_new_box_zeroed_with_elems_dealloc_ub() {
    // `new_box_zeroed_with_elems(1)` allocates Layout { size: 7, align: 1 },
    // constructs `Box<Outer>` with metadata 1 (`size_of_val == 9`), and on drop
    // deallocates with Layout { size: 9, align: 1 }!
    let b: Box<Outer> = Outer::new_box_zeroed_with_elems(1).unwrap();
    assert_eq!(core::mem::size_of_val(&*b), 9);
    drop(b);
}
output.log
=== cargo test (native) ===
     Running tests/poc.rs (target/debug/deps/poc-c08991bf61b840c1)

running 4 tests
test poc_layout_size_mismatch ... Sub-case 1 (elems = 1): zerocopy size = 7, core::mem::size_of_val = 9
Sub-case 2 (elems = 0): zerocopy size = 5, core::mem::size_of_val = 8
ok
test poc_new_box_zeroed_with_elems_dealloc_ub ... ok
test poc_ref_from_bytes_oob_sub_case_1 ... ok
test poc_ref_from_bytes_oob_sub_case_2 ... ok

test result: ok. 4 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s


=== cargo +nightly miri test (poc_ref_from_bytes_oob_sub_case_1) ===
running 1 test
test poc_ref_from_bytes_oob_sub_case_1 ... error: Undefined Behavior: pointer not dereferenceable: pointer must be dereferenceable for 9 bytes, but got alloc40927 which is only 7 bytes from the end of the allocation
   --> library/core/src/ptr/non_null.rs:444:18
    |
444 |         unsafe { &*self.as_ptr().cast_const() }
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
help: alloc40927 was allocated here:
   --> tests/poc.rs:78:9
    |
 78 |     let buf = [0u8; 7];
    |         ^^^
    = note: this is on thread `poc_ref_from_by`
    = note: stack backtrace:
            0: std::ptr::NonNull::<Outer>::as_ref::<'_>
                at library/core/src/ptr/non_null.rs:444:18: 444:46
            1: zerocopy::pointer::ptr::_conversions::<impl zerocopy::Ptr<'_, Outer, (zerocopy::invariant::Shared, zerocopy::invariant::Aligned, zerocopy::invariant::Valid)>>::as_ref
                at zerocopy/src/pointer/ptr.rs:277:22: 277:34
            2: <Outer as zerocopy::FromBytes>::ref_from_bytes
                at zerocopy/src/lib.rs:4161:27: 4161:57
            3: poc_ref_from_bytes_oob_sub_case_1
                at tests/poc.rs:79:17: 79:44
            4: poc_ref_from_bytes_oob_sub_case_1::{closure#0}
                at tests/poc.rs:75:39: 75:39


=== cargo +nightly miri test (poc_ref_from_bytes_oob_sub_case_2) ===
running 1 test
test poc_ref_from_bytes_oob_sub_case_2 ... error: Undefined Behavior: pointer not dereferenceable: pointer must be dereferenceable for 8 bytes, but got alloc40927 which is only 5 bytes from the end of the allocation
   --> library/core/src/ptr/non_null.rs:444:18
    |
444 |         unsafe { &*self.as_ptr().cast_const() }
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
help: alloc40927 was allocated here:
   --> tests/poc.rs:87:9
    |
 87 |     let buf = [0u8; 5];
    |         ^^^
    = note: this is on thread `poc_ref_from_by`
    = note: stack backtrace:
            0: std::ptr::NonNull::<Outer2>::as_ref::<'_>
                at library/core/src/ptr/non_null.rs:444:18: 444:46
            1: zerocopy::pointer::ptr::_conversions::<impl zerocopy::Ptr<'_, Outer2, (zerocopy::invariant::Shared, zerocopy::invariant::Aligned, zerocopy::invariant::Valid)>>::as_ref
                at zerocopy/src/pointer/ptr.rs:277:22: 277:34
            2: <Outer2 as zerocopy::FromBytes>::ref_from_bytes
                at zerocopy/src/lib.rs:4161:27: 4161:57
            3: poc_ref_from_bytes_oob_sub_case_2
                at tests/poc.rs:88:18: 88:46
            4: poc_ref_from_bytes_oob_sub_case_2::{closure#0}
                at tests/poc.rs:84:39: 84:39


=== cargo +nightly miri test (poc_new_box_zeroed_with_elems_dealloc_ub) ===
running 1 test
test poc_new_box_zeroed_with_elems_dealloc_ub ... error: Undefined Behavior: constructing invalid value of type std::boxed::Box<Outer>: encountered a dangling box (going beyond the bounds of its allocation)
    --> library/alloc/src/boxed.rs:1536:9
     |
1536 |         Box(unsafe { Unique::new_unchecked(raw) }, alloc)
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
     |
     = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
     = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
     = note: this is on thread `poc_new_box_zer`
     = note: stack backtrace:
             0: std::boxed::Box::<Outer>::from_raw_in
                 at library/alloc/src/boxed.rs:1536:9: 1536:58
             1: std::boxed::Box::<Outer>::from_raw
                 at library/alloc/src/boxed.rs:1309:18: 1309:48
             2: zerocopy::util::new_box::<Outer>
                 at zerocopy/src/util/mod.rs:467:17: 467:58
             3: <Outer as zerocopy::FromZeros>::new_box_zeroed_with_elems
                 at zerocopy/src/lib.rs:3707:18: 3707:73
             4: poc_new_box_zeroed_with_elems_dealloc_ub
                 at tests/poc.rs:97:25: 97:60
             5: poc_new_box_zeroed_with_elems_dealloc_ub::{closure#0}
                 at tests/poc.rs:93:46: 93:46
fix.patch
diff --git a/zerocopy/src/layout.rs b/zerocopy/src/layout.rs
index 68a2b7ad2..b3e84d61a 100644
--- a/zerocopy/src/layout.rs
+++ b/zerocopy/src/layout.rs
@@ -428,6 +428,15 @@ impl DstLayout {
                             offset: trailing_offset,
                             elem_size,
                         }) => {
+                            // `#[repr(packed(N))]` on the outer struct reduces
+                            // `field_align`, but does not strip internal
+                            // trailing padding from `field`. If `field`'s
+                            // alignment is reduced by `repr_packed` and `field`
+                            // requires dynamic padding, `DstLayout` cannot
+                            // represent the composite layout.
+                            if field_align.get() < field.align.get() && field.requires_dynamic_padding() {
+                                const_panic!("Cannot pack a dynamically-padded slice DST to a smaller alignment.");
+                            }
                             // If the trailing field is dynamically sized, so too
                             // will the resulting layout. The offset of the trailing
                             // slice component is the sum of the offset of the
@@ -2148,14 +2157,26 @@ mod kani_proofs {
                     // The elem size is unchanged.
                     assert_eq!(composite_elem_size, field_elem_size);
 
-                    let field_analog =
-                        Layout::from_size_align(field_offset, field_align.get()).unwrap();
-
-                    if let Ok((actual_composite, actual_offset)) = base_analog.extend(field_analog)
+                    let elems: usize = kani::any();
+                    if let Some(unpadded_field_size) =
+                        field_elem_size.checked_mul(elems).and_then(|s| s.checked_add(field_offset))
                     {
-                        assert_eq!(actual_offset, offset);
-                        assert_eq!(actual_composite.size(), composite_offset);
-                        assert_eq!(actual_composite.align(), composite.align.get());
+                        if let Ok(padded_field) =
+                            Layout::from_size_align(unpadded_field_size, field.align.get())
+                                .map(|l| l.pad_to_align())
+                        {
+                            let field_analog =
+                                Layout::from_size_align(padded_field.size(), field_align.get())
+                                    .unwrap();
+                            if let Ok((actual_composite, actual_offset)) =
+                                base_analog.extend(field_analog)
+                            {
+                                let actual_composite = actual_composite.pad_to_align();
+                                assert_eq!(actual_offset, offset);
+                                assert_eq!(actual_composite.align(), composite.align.get());
+                                assert_eq!(
+                                    Some(actual_composite.size()),
+                                    elems.size_for_metadata(composite)
+                                );
+                            }
+                        }
                     }
                 } else {
                     panic!("The extension of a layout with a DST must result in a DST.")

Suggested Fix

Because KnownLayout::LAYOUT's safety invariant (zerocopy/src/lib.rs:781-795) requires offset to be the exact byte offset of the trailing slice and (offset + m * elem_size)..size to be trailing padding rounded to LAYOUT.align, DstLayout::extend on a SliceDst field field is valid if and only if:

field_align.get() == field.align.get() || !field.requires_dynamic_padding()
  • When !field.requires_dynamic_padding() (for example, #[repr(C, packed)] struct S { a: u8, b: [u32] } or a packed wrapper around an unpadded #[repr(C)] slice DST), field's size trailing_offset + elem_size * elems is already an exact multiple of field.align for every metadata elems, so field never has internal trailing padding and DstLayout::extend is sound even when field_align < field.align.
  • When field_align.get() < field.align.get() && field.requires_dynamic_padding(), DstLayout cannot represent the outer type's layout and DstLayout::extend should const_panic! at compile time.
  • Update prove_dst_layout_extend in zerocopy/src/layout.rs to model field's slice elements (elems * field_elem_size) and field's own .pad_to_align() (field.align) before applying field_align and base_analog.extend(...).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions