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 8F56C1FF0AA for ; Fri, 21 Aug 2026 16:02:43 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id CBD52215A8; Fri, 21 Aug 2026 16:02:42 +0200 (CEST) From: "Max R. Carrara" To: pbs-devel@lists.proxmox.com Subject: [PATCH proxmox{,-backup} v2 00/10] Fix Undefined Behavior in Tape Block Header Deallocation Date: Fri, 21 Aug 2026 16:02:24 +0200 Message-ID: <20260821140238.615302-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: 1787320931638 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.719 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: GWEWK3R3WO55IZOZPGCML72QRTJMGID6 X-Message-ID-Hash: GWEWK3R3WO55IZOZPGCML72QRTJMGID6 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 - v2 ============================================================= 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 v2 here is a greater rework of v1, so a lot has changed. - Introduce the `proxmox-alloc` crate together with its `LayoutAwareBox` type that keeps track of the layout used during allocation and re-uses it during deallocation. This is the main type that allows us to prevent UB in this series. I was first thinking of keeping this type inside of `pbs-tape`, but after implementing two thirds of it, I figured it's probably better to introduce it as part of a separate crate together with ample tests and documentation instead. This documents the problems the type addresses and also shows how it ought to be used, which might be helpful for developers who rarely touch lower-level Rust code. FYI, the docs of all our crates in proxmox.git can be built and opened using: cargo doc --workspace --no-deps --open - Refactor the tape code in smaller pieces to make reviewing all changes easier. - Move `BlockHeader` into a separate module and rename it to `TapeBlock`, since that's what it actually represents. - Get rid of any `pub` fields in `TapeBlock` and add any necessary getters and setters. - Remove haphazard `unsafe` blocks by implementing them as methods instead. - Add "SAFETY" comments to all `unsafe` blocks that are touched or introduced. - Add tests in `pbs-tape` that allow Miri to easily catch any UB regarding the two instances this patch series addresses. Testing ------- 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/ Summary of Changes ------------------ proxmox: Max R. Carrara (2): proxmox-alloc: introduce proxmox-alloc with `LayoutAwareBox` type proxmox-alloc: document undefined behavior regarding custom allocs Cargo.toml | 1 + proxmox-alloc/Cargo.toml | 24 + proxmox-alloc/examples/ub.rs | 99 ++ proxmox-alloc/src/aware_boxed.rs | 2199 ++++++++++++++++++++++++++++++ proxmox-alloc/src/lib.rs | 19 + 5 files changed, 2342 insertions(+) create mode 100644 proxmox-alloc/Cargo.toml create mode 100644 proxmox-alloc/examples/ub.rs 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 | 191 +++++++++++++++++++++++++++++++++ 7 files changed, 300 insertions(+), 153 deletions(-) create mode 100644 pbs-tape/src/tape_block.rs Summary over all repositories: 12 files changed, 2642 insertions(+), 153 deletions(-) -- Generated by murpp 0.12.0