From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 25A591FF0AF for ; Thu, 10 Sep 2026 17:25:03 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 7055B2142C; Thu, 10 Sep 2026 17:25:01 +0200 (CEST) From: "Max R. Carrara" To: pbs-devel@lists.proxmox.com Subject: [PATCH proxmox{,-backup} v4 0/9] Fix Undefined Behavior in Tape Block Header Deallocation Date: Thu, 10 Sep 2026 17:23:26 +0200 Message-ID: <20260910152454.547620-1-m.carrara@proxmox.com> X-Mailer: git-send-email 2.47.3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1789053885598 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.544 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: B7XSHC4N75NV2A4EY3RZWRCFTM44DLIR X-Message-ID-Hash: B7XSHC4N75NV2A4EY3RZWRCFTM44DLIR X-MailFrom: m.carrara@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Fix Undefined Behavior in Tape Block Header Deallocation - v4 ============================================================= We allocate `BlockHeader` in `pbs-tape` with an alignment equal to the page size, but because it uses packed / 1-byte alignment (`#[repr(C, packed)]` to be precise), the compiler will treat it as if it was allocated with 1-byte alignment. Miri [0] will flag this as undefined behavior: error: Undefined Behavior: incorrect layout on deallocation: alloc46398 has size 262144 and alignment 4096, but gave size 262144 and alignment 1 This is because Rust cares about a memory region's alignment on deallocation, even if the underlying allocator does not -- hence why this hasn't actually been a problem for us. Still, there is no guarantee that this will not change at some point in the future. The same kind of UB is also caused by the `alloc_page_aligned_buffer()` function of the `sgutils2` module. This series addresses both of these issues by introducing a new type called `LayoutAwareBox`, which keeps track of the layout a given heap allocation used. Special thanks to @Robert, who brought this to my attention and totally managed to nerd-snipe me with this. I have added a respective git trailer in each of the patches that address the occurrences of UB. Notable Changes --------------- This is a quick refresh of v3, aiming to fix a couple of cases where `LayoutAwareBox` leaked memory when certain methods panicked. The *only* instance where we still may leak memory is if a drop handler of an element of a slice panics while another panic is already unwinding. In that case, there is not really much we can do without overengineering the data structure. In particular: - In the regular (non-slice) `Clone` impl, a panic when calling `clone()` would leak memory, since the ownership of the pointer would be transferred to the aware-box only after the write. To fix this, add an inline `DropGuard` struct whose drop handler calls `dealloc`. If the write is successful, we `mem::forget()` the drop guard; otherwise, `DropGuard::drop()` frees the allocation before continuing to unwind. Respective tests for this are added to ensure this doesn't happen again. - For sized types, when allocating a new slice using `LayoutAwareBox::<[T]>::slice_fill_with()` and related methods that use it under the hood, a panic in the initialization function would cause the fat pointer tracking the number of initialized elements to be used for the deallocation as well. To be more precise, if 5 out of 10 elements were initialized before panicking, the local `LayoutAwareBox<[T]>` would store a fat pointer with a length of 5. During `LayoutAwareBox::<[T]>::drop()`, this pointer would then be used in the call to `dealloc()`. This is actually undefined behavior once again (according to Miri), since the length of the pointer does not match up with the allocation's size. To fix this, use another inline `DropGuard` struct that tracks the number of initialized elements *in addition to the allocation*. If the initialization loop panics, `DropGuard` creates a fat pointer with the number of initialized elements and passes it to `core::ptr::drop_in_place()`, but passes the *original* pointer (and layout) to `dealloc()`. This prevents UB and also lets Miri correctly reason about our allocations. Like for the previous point, respective tests for this are added here too. Note that these tests need to be run using Miri to actually check for UB. - Another fix for `LayoutAwareBox::<[T]>::slice_fill_with()` makes the method actually call the initializer function for zero-sized types. Add a test case to ensure this is actually done. - Use the size of the stored layout in `LayoutAwareBox` to check whether we have a zero-sized type instead of calling `size_of_val()`. - Toss out `LayoutAwareBox::<[T]>::zero_sized_slice()`, since that's become redundant (and was just confusing anyways). Testing ------- As before, would be great if somebody with a working tape storage could give this a spin. Miri does not report any UB anymore and the tests we have pass, but some smoke-testing would nevertheless be appreciated. My virtual tape library has unfortunately borked itself, and I haven't come around to un-borking it yet. [0]: https://github.com/rust-lang/miri Previous Versions ----------------- v1: https://lore.proxmox.com/pbs-devel/20260805141620.4190773-1-m.carrara@proxmox.com/ v2: https://lore.proxmox.com/pbs-devel/20260821140238.615302-1-m.carrara@proxmox.com/ v3: https://lore.proxmox.com/pbs-devel/20260909154027.595374-1-m.carrara@proxmox.com/ Summary of Changes ------------------ proxmox: Max R. Carrara (1): proxmox-alloc: introduce proxmox-alloc with `LayoutAwareBox` type Cargo.toml | 1 + proxmox-alloc/Cargo.toml | 20 + proxmox-alloc/debian/changelog | 5 + proxmox-alloc/debian/control | 30 + proxmox-alloc/debian/copyright | 18 + proxmox-alloc/debian/debcargo.toml | 7 + proxmox-alloc/src/aware_boxed.rs | 2821 ++++++++++++++++++++++++++++ proxmox-alloc/src/lib.rs | 21 + 8 files changed, 2923 insertions(+) create mode 100644 proxmox-alloc/Cargo.toml create mode 100644 proxmox-alloc/debian/changelog create mode 100644 proxmox-alloc/debian/control create mode 100644 proxmox-alloc/debian/copyright create mode 100644 proxmox-alloc/debian/debcargo.toml create mode 100644 proxmox-alloc/src/aware_boxed.rs create mode 100644 proxmox-alloc/src/lib.rs proxmox-backup: Max R. Carrara (8): tape: move tape block structs into separate file module tape: rename `BlockHeader` and `BlockHeaderFlags` tape: blocked_{reader,writer}: rename `buffer` to `tape_block` tape: tape block: represent tape block header with its own struct tape: tape block: make `payload` field private tape: blocked_{reader,writer}: remove haphazard `unsafe` blocks tape: tape block: fix undefined behavior on tape block deallocation tape: sgutils2: fix undefined behavior in dealloc of buffer Cargo.toml | 3 + pbs-tape/Cargo.toml | 1 + pbs-tape/src/blocked_reader.rs | 72 +++++++------ pbs-tape/src/blocked_writer.rs | 51 +++++---- pbs-tape/src/lib.rs | 86 +++------------ pbs-tape/src/sgutils2.rs | 49 ++++++--- pbs-tape/src/tape_block.rs | 185 +++++++++++++++++++++++++++++++++ 7 files changed, 294 insertions(+), 153 deletions(-) create mode 100644 pbs-tape/src/tape_block.rs Summary over all repositories: 15 files changed, 3217 insertions(+), 153 deletions(-) -- Generated by murpp 0.12.0