[PATCH proxmox-backup v2 09/10] tape: tape block: fix undefined behavior on tape block deallocation

["Max R. Carrara" <m.carrara@proxmox.com>]

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<T>` type of
`proxmox-alloc` and return a `LayoutAwareBox<TapeBlock>` instead of a
plain `Box<TapeBlock>` 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::<TapeBlockHeader>()` instead of hard-coding its (expected)
size.

Update the two instances where we allocate tape blocks. Since
`LayoutAwareBox<T>` works similar to `Box<T>`, 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 <r.obkircher@proxmox.com>
Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
---
 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<R> {
     reader: R,
-    tape_block: Box<TapeBlock>,
+    tape_block: LayoutAwareBox<TapeBlock>,
     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<W: BlockWrite> {
     writer: W,
-    tape_block: Box<TapeBlock>,
+    tape_block: LayoutAwareBox<TapeBlock>,
     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<usize> = 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<Self> {
-        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<Self> {
+        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::<TapeBlockHeader>();
+        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<TapeBlock>`.
+    #[test]
+    fn miri_check_dealloc() {
+        let tape_block = TapeBlock::new();
+        drop(tape_block);
+    }
+}
-- 
2.47.3