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 272141FF0AA for ; Fri, 21 Aug 2026 16:03:18 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 0498D215DD; Fri, 21 Aug 2026 16:03:14 +0200 (CEST) From: "Max R. Carrara" To: pbs-devel@lists.proxmox.com Subject: [PATCH proxmox-backup v2 09/10] tape: tape block: fix undefined behavior on tape block deallocation Date: Fri, 21 Aug 2026 16:02:33 +0200 Message-ID: <20260821140238.615302-10-m.carrara@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260821140238.615302-1-m.carrara@proxmox.com> References: <20260821140238.615302-1-m.carrara@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1787320950517 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.650 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: ZX5X4XN24LRERARDOBF5CSGTF3BHKMSE X-Message-ID-Hash: ZX5X4XN24LRERARDOBF5CSGTF3BHKMSE 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: The `TapeBlock` struct in `pbs-tape` is a dynamically sized type whose data we allocate with an alignment [align] equal to the page size. However, since `TapeBlock` uses `#[repr(C, packed)]`, the Rust compiler will always treat the type as having an alignment of 1, since it cannot (ever) track which memory layout was used for any given heap allocation of `T`. This means that on deallocation, Rust will deallocate a `TapeBlock` with an alignment of 1 instead of the page size alignment that was used to allocate it. This mismatch in alignment on alloc / dealloc is undefined behavior [ub] as reported by Miri [miri]. Note that for *nix and WASM, this does currently not have any known impact, since allocators on these targets do not actually care about alignment on deallocation. There is however no guarantee that this will not change in the future. If we were to target Windows, it would already be a problem [alloc-win]. To solve this, use the newly introduced `LayoutAwareBox` type of `proxmox-alloc` and return a `LayoutAwareBox` instead of a plain `Box` from `TapeBlock::new()`. `LayoutAwareBox` tracks the layout that was used during allocation and uses it for deallocation as well, which fixes the aforementioned undefined behavior occurring here. Besides that, rework the implementation of `TapeBlock::new()` and call `std::alloc::handle_alloc_error` if allocating a new tape block fails. Add a SAFETY comment to each `unsafe` block as well. Use `size_of::()` instead of hard-coding its (expected) size. Update the two instances where we allocate tape blocks. Since `LayoutAwareBox` works similar to `Box`, only the type signature needs to be changed. Also, use a crate-public `LazyLock`ed static for keeping track of the page size. Since the page size is a system constant [sysconf], this is safe to do. Finally, add a simple test for `TapeBlock` that allows Miri to check whether there is any UB on deallocation. [align]: https://en.wikipedia.org/wiki/Data_structure_alignment [alloc-win]: https://github.com/rust-lang/rust/blob/c9ff496891c278ad660bc0ab85c1f0b72059464a/library/std/src/sys/alloc/windows.rs#L182 [miri]: https://github.com/rust-lang/miri [sysconf]: `man 3 sysconf` [ub]: https://doc.rust-lang.org/reference/behavior-considered-undefined.html Reported-by: Robert Obkircher Signed-off-by: Max R. Carrara --- Cargo.toml | 3 ++ pbs-tape/Cargo.toml | 1 + pbs-tape/src/blocked_reader.rs | 4 ++- pbs-tape/src/blocked_writer.rs | 3 +- pbs-tape/src/lib.rs | 13 ++++++++ pbs-tape/src/tape_block.rs | 60 ++++++++++++++++++++++++++++------ 6 files changed, 72 insertions(+), 12 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index f3b67ba79..933c9afd0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -53,6 +53,7 @@ path = "src/lib.rs" [workspace.dependencies] # proxmox workspace +proxmox-alloc = "0.1.0" proxmox-apt = { version = "1.0", features = [ "cache" ] } proxmox-apt-api-types = "3.0" proxmox-async = "0.5" @@ -217,6 +218,7 @@ zstd.workspace = true #valgrind_request = { git = "https://github.com/edef1c/libvalgrind_request", version = "1.1.0", optional = true } # proxmox workspace +proxmox-alloc.workspace = true proxmox-apt.workspace = true proxmox-apt-api-types.workspace = true proxmox-async.workspace = true @@ -282,6 +284,7 @@ proxmox-rrd-api-types.workspace = true #pbs-api-types = { path = "../proxmox/pbs-api-types" } #proxmox-acme = { path = "../proxmox/proxmox-acme" } #proxmox-acme-api = { path = "../proxmox/proxmox-acme-api" } +#proxmox-alloc = { path = "../proxmox/proxmox-alloc" } #proxmox-api-macro = { path = "../proxmox/proxmox-api-macro" } #proxmox-apt = { path = "../proxmox/proxmox-apt" } #proxmox-apt-api-types = { path = "../proxmox/proxmox-apt-api-types" } diff --git a/pbs-tape/Cargo.toml b/pbs-tape/Cargo.toml index 4f153feda..6007b683f 100644 --- a/pbs-tape/Cargo.toml +++ b/pbs-tape/Cargo.toml @@ -21,6 +21,7 @@ serde_json.workspace = true thiserror.workspace = true udev.workspace = true +proxmox-alloc.workspace = true proxmox-io.workspace = true proxmox-lang.workspace=true proxmox-log.workspace=true diff --git a/pbs-tape/src/blocked_reader.rs b/pbs-tape/src/blocked_reader.rs index 6f5f87aa7..3d5cc8f12 100644 --- a/pbs-tape/src/blocked_reader.rs +++ b/pbs-tape/src/blocked_reader.rs @@ -1,5 +1,7 @@ use std::io::Read; +use proxmox_alloc::LayoutAwareBox; + use crate::{ BlockRead, BlockReadError, PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0, TapeBlock, TapeBlockFlags, TapeRead, @@ -18,7 +20,7 @@ use crate::{ /// the end of the stream). pub struct BlockedReader { reader: R, - tape_block: Box, + tape_block: LayoutAwareBox, seq_nr: u32, found_end_marker: bool, incomplete: bool, diff --git a/pbs-tape/src/blocked_writer.rs b/pbs-tape/src/blocked_writer.rs index 44ff15ae0..39ac22a10 100644 --- a/pbs-tape/src/blocked_writer.rs +++ b/pbs-tape/src/blocked_writer.rs @@ -1,3 +1,4 @@ +use proxmox_alloc::LayoutAwareBox; use proxmox_io::vec; use crate::{BlockWrite, TapeBlock, TapeBlockFlags, TapeWrite}; @@ -9,7 +10,7 @@ use crate::{BlockWrite, TapeBlock, TapeBlockFlags, TapeWrite}; /// to the underlying writer. pub struct BlockedWriter { writer: W, - tape_block: Box, + tape_block: LayoutAwareBox, buffer_pos: usize, seq_nr: u32, logical_end_of_media: bool, diff --git a/pbs-tape/src/lib.rs b/pbs-tape/src/lib.rs index 1b6dc94cc..d4a03af6b 100644 --- a/pbs-tape/src/lib.rs +++ b/pbs-tape/src/lib.rs @@ -51,6 +51,19 @@ pub const PROXMOX_BACKUP_MEDIA_LABEL_MAGIC_1_0: [u8; 8] = [42, 5, 191, 60, 176, // openssl::sha::sha256(b"Proxmox Backup MediaSet Label v1.0") pub const PROXMOX_BACKUP_MEDIA_SET_LABEL_MAGIC_1_0: [u8; 8] = [8, 96, 99, 249, 47, 151, 83, 216]; +pub(crate) static PAGE_SIZE: std::sync::LazyLock = std::sync::LazyLock::new(|| { + // See `man 3 sysconf` -- this is a constant and does therefore not change + // during the lifetime of a process. + // SAFETY: Should always be safe to call, and we check for errors afterwards. + let page_size = unsafe { libc::sysconf(libc::_SC_PAGESIZE) }; + + if page_size <= 0 { + panic!("failed to query PAGESIZE ({page_size})"); + } + + page_size as usize +}); + #[derive(Endian, Copy, Clone, Debug)] #[repr(C, packed)] /// Media Content Header diff --git a/pbs-tape/src/tape_block.rs b/pbs-tape/src/tape_block.rs index c6ff98395..4ff91bdc0 100644 --- a/pbs-tape/src/tape_block.rs +++ b/pbs-tape/src/tape_block.rs @@ -1,7 +1,9 @@ use bitflags::bitflags; +use proxmox_alloc::LayoutAwareBox; use proxmox_lang::static_assert_size; +use crate::PAGE_SIZE; use crate::PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0; use crate::PROXMOX_TAPE_BLOCK_SIZE; @@ -50,19 +52,44 @@ bitflags! { impl TapeBlock { pub const SIZE: usize = PROXMOX_TAPE_BLOCK_SIZE; - /// Allocates a new instance on the heap - pub fn new() -> Box { - use std::alloc::{Layout, alloc_zeroed}; + /// Allocates a new [`TapeBlock`] on the heap. + /// + /// [`LayoutAwareBox`] ensures that the layout that was used for allocation + /// is also used for dealloction. + pub fn new() -> LayoutAwareBox { + use std::alloc::Layout; + use std::alloc::alloc_zeroed; + use std::alloc::handle_alloc_error; // align to PAGESIZE, so that we can use it with SG_IO - let page_size = unsafe { libc::sysconf(libc::_SC_PAGESIZE) } as usize; + let layout = Layout::from_size_align(Self::SIZE, *PAGE_SIZE).expect("infallible"); - let mut buffer = unsafe { - let ptr = alloc_zeroed(Layout::from_size_align(Self::SIZE, page_size).unwrap()); - Box::from_raw(core::ptr::slice_from_raw_parts_mut(ptr, Self::SIZE - 16) as *mut Self) - }; - buffer.header.magic = PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0; - buffer + // SAFETY: layout always has a size > 0. Other size and alignment checks + // were performed by from_size_align above. + let thin_ptr = unsafe { alloc_zeroed(layout) }; + if thin_ptr.is_null() { + handle_alloc_error(layout); + } + + let payload_len = Self::SIZE - size_of::(); + let fat_ptr = core::ptr::slice_from_raw_parts_mut(thin_ptr, payload_len) as *mut Self; + + // SAFETY: + // - We checked that `thin_ptr` isn't null above, so `fat_ptr` isn't + // null either. + // - We converted `thin_ptr` to `fat_ptr` with the correct payload + // length, which is the total size minus the header's size. + // - `fat_ptr` is valid for reading and writing. + // - `fat_ptr` is not aliased anywhere. + // - The memory `fat_ptr` points to was zero-initialized by + // `alloc_zeroed` earlier. + // - The value that `fat_ptr` points to has the same size as its + // allocation. + let mut tape_block = unsafe { LayoutAwareBox::from_raw_parts(fat_ptr, layout) }; + + tape_block.header.magic = PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0; + + tape_block } /// Returns the magic value of this tape block's header. @@ -149,3 +176,16 @@ impl TapeBlock { unsafe { std::slice::from_raw_parts_mut((self as *mut _) as *mut u8, Self::SIZE) } } } + +#[cfg(test)] +mod test { + use super::*; + + // Test for Miri to check whether there's any memory layout mismatch during + // the deallocation of a `LayoutAwareBox`. + #[test] + fn miri_check_dealloc() { + let tape_block = TapeBlock::new(); + drop(tape_block); + } +} -- 2.47.3