[PATCH proxmox-backup v2 10/10] tape: sgutils2: fix undefined behavior in dealloc of buffer

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

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::<u8>()` (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 <r.obkircher@proxmox.com>
Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
---
 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<Box<[u8]>, 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<LayoutAwareBox<[u8]>, 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<Self, Error> {
-        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<F: AsRawFd>(file: &mut F) -> Result<RequestSenseFixed,
 
 #[cfg(test)]
 mod test {
+    use crate::PAGE_SIZE;
+    use crate::sgutils2::alloc_page_aligned_buffer;
     use crate::sgutils2::scsi_ascii_to_string;
 
+    #[test]
+    fn page_aligned_buffer_zero_length() {
+        let buf = alloc_page_aligned_buffer(0).expect("infallible");
+
+        assert_eq!(buf.len(), 0);
+    }
+
+    #[test]
+    fn page_aligned_buffer_alignment() {
+        let len = 8 * 1024;
+        let buf = alloc_page_aligned_buffer(len).expect("infallible");
+
+        assert_eq!(buf.len(), len);
+
+        let page_size = *PAGE_SIZE;
+        assert_eq!(
+            (buf.as_ptr() as usize) & (page_size - 1),
+            0,
+            "not actually aligned to page size"
+        );
+    }
+
     #[test]
     fn test_scsi_ascii_to_string() {
         fn test(input: &'static str, expected: &'static str) {
-- 
2.47.3