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 214511FF0AA for ; Fri, 21 Aug 2026 16:03:15 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 90A55215FC; Fri, 21 Aug 2026 16:03:13 +0200 (CEST) From: "Max R. Carrara" To: pbs-devel@lists.proxmox.com Subject: [PATCH proxmox-backup v2 10/10] tape: sgutils2: fix undefined behavior in dealloc of buffer Date: Fri, 21 Aug 2026 16:02:34 +0200 Message-ID: <20260821140238.615302-11-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: 1787320952608 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.656 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: PAJAVT7LSWXHUVSWY43NCKAPGKREUMWG X-Message-ID-Hash: PAJAVT7LSWXHUVSWY43NCKAPGKREUMWG 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 `alloc_page_aligned_buffer()` function in the `sgutils2` module manually allocates a page size aligned buffer. However, there is no way for Rust to track the alignment that was used for any given heap allocation. This means that on deallocation, Rust will deallocate any such buffer using its element type's alignment, which in this case is `align_of::()` (1). 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]. Solve this by using the `LayoutAwareBox` from `proxmox-alloc` and returning it instead of the plain `Box`. Since `LayoutAwareBox` also handles requests to allocate zero-length slices, pre-emptive checks whether the requested buffer size is 0 are not necessary anymore. In other words, `alloc_page_aligned_buffer()` will now correctly return a zero-length buffer if the requested size is 0, instead of causing even more undefined behavior -- passing a `Layout` with size 0 to `alloc()` & Co. is also UB [alloc-safety]! Also use the newly introduced `PAGE_SIZE` static to fetch the page size where we are currently querying the page size via `libc::sysconf()`. [align]: https://en.wikipedia.org/wiki/Data_structure_alignment [alloc-safety]: https://doc.rust-lang.org/alloc/alloc/trait.GlobalAlloc.html#safety-3 [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 [ub]: https://doc.rust-lang.org/reference/behavior-considered-undefined.html Reported-by: Robert Obkircher Signed-off-by: Max R. Carrara --- pbs-tape/src/sgutils2.rs | 49 ++++++++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 17 deletions(-) diff --git a/pbs-tape/src/sgutils2.rs b/pbs-tape/src/sgutils2.rs index 340616c73..ab02c6ad2 100644 --- a/pbs-tape/src/sgutils2.rs +++ b/pbs-tape/src/sgutils2.rs @@ -15,8 +15,11 @@ use endian_trait::Endian; use libc::{c_char, c_int}; use serde::{Deserialize, Serialize}; +use proxmox_alloc::LayoutAwareBox; use proxmox_io::ReadExt; +use crate::PAGE_SIZE; + #[derive(thiserror::Error, Debug)] pub struct SenseInfo { pub sense_key: u8, @@ -405,7 +408,7 @@ extern "C" { /// Safe interface to run RAW SCSI commands pub struct SgRaw<'a, F> { file: &'a mut F, - buffer: Box<[u8]>, + buffer: LayoutAwareBox<[u8]>, sense_buffer: [u8; 32], timeout: i32, } @@ -435,16 +438,8 @@ pub fn get_asc_ascq_string(asc: u8, ascq: u8) -> String { /// Allocate a page aligned buffer /// /// SG RAWIO commands needs page aligned transfer buffers. -pub fn alloc_page_aligned_buffer(buffer_size: usize) -> Result, Error> { - let page_size = unsafe { libc::sysconf(libc::_SC_PAGESIZE) } as usize; - let layout = std::alloc::Layout::from_size_align(buffer_size, page_size)?; - let dinp = unsafe { std::alloc::alloc_zeroed(layout) }; - if dinp.is_null() { - bail!("alloc SCSI output buffer failed"); - } - - let buffer_slice = unsafe { std::slice::from_raw_parts_mut(dinp, buffer_size) }; - Ok(unsafe { Box::from_raw(buffer_slice) }) +pub fn alloc_page_aligned_buffer(buffer_size: usize) -> Result, Error> { + LayoutAwareBox::slice_fill(buffer_size, *PAGE_SIZE, 0).map_err(Error::from) } impl<'a, F: AsRawFd> SgRaw<'a, F> { @@ -452,11 +447,7 @@ impl<'a, F: AsRawFd> SgRaw<'a, F> { /// /// The file must be a handle to a SCSI device. pub fn new(file: &'a mut F, buffer_size: usize) -> Result { - let buffer = if buffer_size > 0 { - alloc_page_aligned_buffer(buffer_size)? - } else { - Box::new([]) - }; + let buffer = alloc_page_aligned_buffer(buffer_size)?; let sense_buffer = [0u8; 32]; @@ -674,7 +665,7 @@ impl<'a, F: AsRawFd> SgRaw<'a, F> { return Err(format_err!("no valid SCSI command").into()); } - let page_size = unsafe { libc::sysconf(libc::_SC_PAGESIZE) } as usize; + let page_size = *PAGE_SIZE; if ((data.as_ptr() as usize) & (page_size - 1)) != 0 { return Err(format_err!("wrong transfer buffer alignment").into()); } @@ -1017,8 +1008,32 @@ pub fn scsi_request_sense(file: &mut F) -> Result