From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 6EE901FF0AB for ; Wed, 09 Sep 2026 17:40:33 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 23CD9215C9; Wed, 09 Sep 2026 17:40:33 +0200 (CEST) From: "Max R. Carrara" To: pbs-devel@lists.proxmox.com Subject: [PATCH proxmox{,-backup} v3 0/9] Fix Undefined Behavior in Tape Block Header Deallocation Date: Wed, 9 Sep 2026 17:40:14 +0200 Message-ID: <20260909154027.595374-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: 1788968419356 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.598 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: NL3XS26EPOE3JNK23EJBGDHKHHEKDZ7Y X-Message-ID-Hash: NL3XS26EPOE3JNK23EJBGDHKHHEKDZ7Y 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 - v3 ============================================================= 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 refresh incorporates the feedback @Robert has given on v2. Many thanks for the review, it's really appreciated! The most noteworthy changes are found below. proxmox: - Drop the inner enum that was used to differ between "known" and "dynamic" layouts -- always store a `std::alloc::Layout` and handle zero-sized types explicitly instead - Fix the `Clone` impls of `LayoutAwareBox` by allocating the new memory with the same layout as that of `self`, then clone and write directly to that memory - Introduce a new method to `LayoutAwareBox` called `from_slice_align()` that allows the caller to easily create an aware box from a given `&[T]` with a particular alignment --> This also happens to be useful for the `Clone` impl for `LayoutAwareBox<[T]>` - Prevent leaking / observing (uninitialized) memory on panic in `slice_fill()` and `slice_fill_with()` of `LayoutAwareBox` by constructing the aware-boxed slice with a length of 0 first, and then updating the length of the allocated slice at the end of every iteration. This also means that drop-handlers of already initialized elements will get called. - Add initial Debian packaging for the proxmox-alloc crate - Drop the `examples/` directory - Adapt SAFETY comments as per @Robert's feedback - Adapt existing tests and add additional ones, such as for `From` and `Clone` impls proxmox-backup: - Adapt docstring of the `TapeBlock` type - Adapt SAFETY comments as per @Robert's feedback 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/ v2: https://lore.proxmox.com/pbs-devel/20260821140238.615302-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 | 2548 ++++++++++++++++++++++++++++ proxmox-alloc/src/lib.rs | 21 + 8 files changed, 2650 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, 2944 insertions(+), 153 deletions(-) -- Generated by murpp 0.12.0