bugfix: passthrough: refect CFileHandle struct

Previously, CFileHandle uses a *mut libc::c_char to transfer data
between user space and kernel space. The system call "name_to_handle_at"
will return the data with "copy_to_user". This may cause a bug because
the memory layout of CFileHandle fields may be noncontinuous with the
dynamically-sized array's. Therefore, the "copy_to_user" may destroy the
stack. This is reproduced on aarch64 only when using "opt-level=3" to
compile.

This commit refectors the CFileHandle struct with FarmStruct trait to
ensure the memory layout to be continuous. The trait enables struct to
own a dynamically-sized array at the end of the struct like zero-array
in C language. We refector the related implementation so that
"copy_to_user" won't write outside the CFileHandle and destroy the user
stack.

Besides, add some units and fix clippy warnings.

Signed-off-by: xuejun-xj <[email protected]>

Author: xuejun-xj

Cargo.toml

@@ -49,7 +49,7 @@ vm-memory = { version = "0.9", features = ["backend-mmap", "backend-bitmap"] }
 default = ["fusedev"]
 async-io = ["async-trait", "tokio-uring", "tokio/fs", "tokio/net", "tokio/sync", "tokio/rt", "tokio/macros", "io-uring"]
 fusedev = ["vmm-sys-util", "caps", "core-foundation-sys"]
-virtiofs = ["virtio-queue", "caps"]
+virtiofs = ["virtio-queue", "caps", "vmm-sys-util"]
 vhost-user-fs = ["virtiofs", "vhost", "caps"]
 
 [package.metadata.docs.rs]

src/passthrough/file_handle.rs

@@ -9,34 +9,81 @@ use std::fmt::{Debug, Formatter};
 use std::fs::File;
 use std::io;
 use std::os::unix::io::{AsRawFd, FromRawFd, RawFd};
-use std::ptr;
 use std::sync::{RwLock, RwLockReadGuard, RwLockWriteGuard};
 
+use vmm_sys_util::{
+    fam::{FamStruct, FamStructWrapper},
+    generate_fam_struct_impl,
+};
+
 use crate::passthrough::PassthroughFs;
 
 /// An arbitrary maximum size for CFileHandle::f_handle.
 ///
 /// According to Linux ABI, struct file_handle has a flexible array member 'f_handle', but it's
 /// hard-coded here for simplicity.
-//pub const MAX_HANDLE_SZ: usize = 128;
+pub const MAX_HANDLE_SIZE: usize = 128;
+
+/// Dynamically allocated array.
+#[derive(Default)]
+#[repr(C)]
+pub struct __IncompleteArrayField<T>(::std::marker::PhantomData<T>, [T; 0]);
+impl<T> __IncompleteArrayField<T> {
+    #[inline]
+    pub unsafe fn as_ptr(&self) -> *const T {
+        self as *const __IncompleteArrayField<T> as *const T
+    }
+    #[inline]
+    pub unsafe fn as_mut_ptr(&mut self) -> *mut T {
+        self as *mut __IncompleteArrayField<T> as *mut T
+    }
+    #[inline]
+    pub unsafe fn as_slice(&self, len: usize) -> &[T] {
+        ::std::slice::from_raw_parts(self.as_ptr(), len)
+    }
+    #[inline]
+    pub unsafe fn as_mut_slice(&mut self, len: usize) -> &mut [T] {
+        ::std::slice::from_raw_parts_mut(self.as_mut_ptr(), len)
+    }
+}
 
-#[derive(Clone, Copy)]
+/// The structure to transfer file_handle struct between user space and kernel space.
+/// ```c
+/// struct file_handle {
+///     __u32 handle_bytes;
+///     int handle_type;
+///     /* file identifier */
+///     unsigned char f_handle[];
+/// }
+/// ```
+#[derive(Default)]
 #[repr(C)]
+pub struct CFileHandleInner {
+    pub handle_bytes: libc::c_uint,
+    pub handle_type: libc::c_int,
+    pub f_handle: __IncompleteArrayField<libc::c_char>,
+}
+
+generate_fam_struct_impl!(
+    CFileHandleInner,
+    libc::c_char,
+    f_handle,
+    libc::c_uint,
+    handle_bytes,
+    MAX_HANDLE_SIZE
+);
+
+type CFileHandleWrapper = FamStructWrapper<CFileHandleInner>;
+
+#[derive(Clone)]
 struct CFileHandle {
-    // Size of f_handle [in, out]
-    handle_bytes: libc::c_uint,
-    // Handle type [out]
-    handle_type: libc::c_int,
-    // File identifier (sized by caller) [out]
-    f_handle: *mut libc::c_char,
+    pub wrapper: CFileHandleWrapper,
 }
 
 impl CFileHandle {
-    fn new() -> Self {
+    fn new(size: usize) -> Self {
         CFileHandle {
-            handle_bytes: 0,
-            handle_type: 0,
-            f_handle: ptr::null_mut(),
+            wrapper: CFileHandleWrapper::new(size).unwrap(),
         }
     }
 }
@@ -47,14 +94,24 @@ unsafe impl Sync for CFileHandle {}
 
 impl Ord for CFileHandle {
     fn cmp(&self, other: &Self) -> Ordering {
-        if self.handle_bytes != other.handle_bytes {
-            return self.handle_bytes.cmp(&other.handle_bytes);
+        let s_fh = self.wrapper.as_fam_struct_ref();
+        let o_fh = other.wrapper.as_fam_struct_ref();
+        if s_fh.handle_bytes != o_fh.handle_bytes {
+            return s_fh.handle_bytes.cmp(&o_fh.handle_bytes);
         }
-        if self.handle_type != other.handle_type {
-            return self.handle_type.cmp(&other.handle_type);
+        let length = s_fh.handle_bytes as usize;
+        if s_fh.handle_type != o_fh.handle_type {
+            return s_fh.handle_type.cmp(&o_fh.handle_type);
+        }
+        unsafe {
+            if s_fh.f_handle.as_ptr() != o_fh.f_handle.as_ptr() {
+                return s_fh
+                    .f_handle
+                    .as_slice(length)
+                    .cmp(o_fh.f_handle.as_slice(length));
+            }
         }
 
-        // f_handle is left to be compared by FileHandle's buf.
         Ordering::Equal
     }
 }
@@ -75,10 +132,11 @@ impl Eq for CFileHandle {}
 
 impl Debug for CFileHandle {
     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        let fh = self.wrapper.as_fam_struct_ref();
         write!(
             f,
             "File handle: type {}, len {}",
-            self.handle_type, self.handle_bytes
+            fh.handle_type, fh.handle_bytes
         )
     }
 }
@@ -88,15 +146,13 @@ impl Debug for CFileHandle {
 pub struct FileHandle {
     pub(crate) mnt_id: u64,
     handle: CFileHandle,
-    // internal buffer for handle.f_handle
-    buf: Vec<libc::c_char>,
 }
 
 extern "C" {
     fn name_to_handle_at(
         dirfd: libc::c_int,
         pathname: *const libc::c_char,
-        file_handle: *mut CFileHandle,
+        file_handle: *mut CFileHandleInner,
         mount_id: *mut libc::c_int,
         flags: libc::c_int,
     ) -> libc::c_int;
@@ -105,7 +161,7 @@ extern "C" {
     // not to change it, so we can declare it `const`.
     fn open_by_handle_at(
         mount_fd: libc::c_int,
-        file_handle: *const CFileHandle,
+        file_handle: *const CFileHandleInner,
         flags: libc::c_int,
     ) -> libc::c_int;
 }
@@ -114,7 +170,7 @@ impl FileHandle {
     /// Create a file handle for the given file.
     pub fn from_name_at(dir_fd: RawFd, path: &CStr) -> io::Result<Self> {
         let mut mount_id: libc::c_int = 0;
-        let mut c_fh = CFileHandle::new();
+        let mut c_fh = CFileHandle::new(0);
 
         // Per name_to_handle_at(2), the caller can discover the required size
         // for the file_handle structure by making a call in which
@@ -126,7 +182,7 @@ impl FileHandle {
             name_to_handle_at(
                 dir_fd,
                 path.as_ptr(),
-                &mut c_fh,
+                c_fh.wrapper.as_mut_fam_struct_ptr(),
                 &mut mount_id,
                 libc::AT_EMPTY_PATH,
             )
@@ -141,18 +197,14 @@ impl FileHandle {
             return Err(io::Error::from(io::ErrorKind::InvalidData));
         }
 
-        let needed = c_fh.handle_bytes as usize;
-        let mut buf = vec![0; needed];
-
-        // get a unsafe pointer, FileHandle takes care of freeing the memory
-        // that 'f_handle' points to.
-        c_fh.f_handle = buf.as_mut_ptr();
+        let needed = c_fh.wrapper.as_fam_struct_ref().handle_bytes as usize;
+        let mut c_fh = CFileHandle::new(needed);
 
         let ret = unsafe {
             name_to_handle_at(
                 dir_fd,
                 path.as_ptr(),
-                &mut c_fh,
+                c_fh.wrapper.as_mut_fam_struct_ptr(),
                 &mut mount_id,
                 libc::AT_EMPTY_PATH,
             )
@@ -161,7 +213,6 @@ impl FileHandle {
             Ok(FileHandle {
                 mnt_id: mount_id as u64,
                 handle: c_fh,
-                buf,
             })
         } else {
             let e = io::Error::last_os_error();
@@ -200,7 +251,13 @@ impl FileHandle {
     /// `mount_fd` must be an open non-`O_PATH` file descriptor for an inode on the same mount as
     /// the file to be opened, i.e. the mount given by `self.mnt_id`.
     fn open(&self, mount_fd: &impl AsRawFd, flags: libc::c_int) -> io::Result<File> {
-        let ret = unsafe { open_by_handle_at(mount_fd.as_raw_fd(), &self.handle, flags) };
+        let ret = unsafe {
+            open_by_handle_at(
+                mount_fd.as_raw_fd(),
+                self.handle.wrapper.as_fam_struct_ptr(),
+                flags,
+            )
+        };
         if ret >= 0 {
             // Safe because `open_by_handle_at()` guarantees this is a valid fd
             let file = unsafe { File::from_raw_fd(ret) };
@@ -330,82 +387,92 @@ impl MountFds {
 mod tests {
     use super::*;
 
+    fn generate_c_file_handle(
+        handle_bytes: usize,
+        handle_type: libc::c_int,
+        buf: Vec<libc::c_char>,
+    ) -> CFileHandle {
+        let mut wrapper = CFileHandle::new(handle_bytes);
+        let fh = wrapper.wrapper.as_mut_fam_struct();
+        fh.handle_type = handle_type;
+        unsafe {
+            fh.f_handle
+                .as_mut_slice(handle_bytes)
+                .copy_from_slice(buf.as_slice());
+        }
+
+        wrapper
+    }
+
     #[test]
     fn test_file_handle_derives() {
-        let mut buf1 = vec![0; 128];
-        let h1 = CFileHandle {
-            handle_bytes: 128,
-            handle_type: 3,
-            f_handle: buf1.as_mut_ptr(),
-        };
+        let h1 = generate_c_file_handle(128, 3, vec![0; 128]);
         let mut fh1 = FileHandle {
             mnt_id: 0,
             handle: h1,
-            buf: buf1,
         };
 
-        let mut buf2 = vec![0; 129];
-        let h2 = CFileHandle {
-            handle_bytes: 129,
-            handle_type: 3,
-            f_handle: buf2.as_mut_ptr(),
-        };
+        let h2 = generate_c_file_handle(127, 3, vec![0; 127]);
         let fh2 = FileHandle {
             mnt_id: 0,
             handle: h2,
-            buf: buf2,
         };
 
-        let mut buf3 = vec![0; 128];
-        let h3 = CFileHandle {
-            handle_bytes: 128,
-            handle_type: 4,
-            f_handle: buf3.as_mut_ptr(),
-        };
+        let h3 = generate_c_file_handle(128, 4, vec![0; 128]);
         let fh3 = FileHandle {
             mnt_id: 0,
             handle: h3,
-            buf: buf3,
         };
 
-        let mut buf4 = vec![1; 128];
-        let h4 = CFileHandle {
-            handle_bytes: 128,
-            handle_type: 3,
-            f_handle: buf4.as_mut_ptr(),
-        };
+        let h4 = generate_c_file_handle(128, 3, vec![1; 128]);
         let fh4 = FileHandle {
             mnt_id: 0,
             handle: h4,
-            buf: buf4,
         };
 
-        let mut buf5 = vec![0; 128];
-        let h5 = CFileHandle {
-            handle_bytes: 128,
-            handle_type: 3,
-            f_handle: buf5.as_mut_ptr(),
-        };
+        let h5 = generate_c_file_handle(128, 3, vec![0; 128]);
         let mut fh5 = FileHandle {
             mnt_id: 0,
             handle: h5,
-            buf: buf5,
         };
 
-        assert!(fh1 < fh2);
+        assert!(fh1 > fh2);
         assert!(fh1 != fh2);
         assert!(fh1 < fh3);
         assert!(fh1 != fh3);
         assert!(fh1 < fh4);
         assert!(fh1 != fh4);
-
         assert!(fh1 == fh5);
-        fh1.buf[0] = 1;
-        fh1.handle.f_handle = fh1.buf.as_mut_ptr();
-        assert!(fh1 > fh5);
 
-        fh5.buf[0] = 1;
-        fh5.handle.f_handle = fh5.buf.as_mut_ptr();
+        unsafe {
+            fh1.handle
+                .wrapper
+                .as_mut_fam_struct()
+                .f_handle
+                .as_mut_slice(128)[0] = 1;
+        }
+        assert!(fh1 > fh5);
+        unsafe {
+            fh5.handle
+                .wrapper
+                .as_mut_fam_struct()
+                .f_handle
+                .as_mut_slice(128)[0] = 1;
+        }
         assert!(fh1 == fh5);
     }
+
+    #[test]
+    fn test_c_file_handle_wrapper() {
+        let buf = (0..=127).collect::<Vec<libc::c_char>>();
+        let mut wrapper = generate_c_file_handle(MAX_HANDLE_SIZE, 3, buf.clone());
+        let fh = wrapper.wrapper.as_mut_fam_struct();
+
+        assert_eq!(fh.handle_bytes as usize, MAX_HANDLE_SIZE);
+        assert_eq!(fh.handle_type, 3);
+        assert_eq!(
+            unsafe { fh.f_handle.as_slice(MAX_HANDLE_SIZE) },
+            buf.as_slice(),
+        );
+    }
 }