From 7fa4f725c120824b36e713548f16eaf6ea413f8e Mon Sep 17 00:00:00 2001 From: xuejun-xj Date: Thu, 23 Feb 2023 11:28:21 +0800 Subject: [PATCH] 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 --- Cargo.toml | 2 +- src/passthrough/file_handle.rs | 219 +++++++++++++++++++++------------ 2 files changed, 144 insertions(+), 77 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 195986b93..57587ad52 100644 --- a/Cargo.toml +++ b/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] diff --git a/src/passthrough/file_handle.rs b/src/passthrough/file_handle.rs index 519e023a3..e0f611891 100644 --- a/src/passthrough/file_handle.rs +++ b/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(::std::marker::PhantomData, [T; 0]); +impl __IncompleteArrayField { + #[inline] + pub unsafe fn as_ptr(&self) -> *const T { + self as *const __IncompleteArrayField as *const T + } + #[inline] + pub unsafe fn as_mut_ptr(&mut self) -> *mut T { + self as *mut __IncompleteArrayField 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, +} + +generate_fam_struct_impl!( + CFileHandleInner, + libc::c_char, + f_handle, + libc::c_uint, + handle_bytes, + MAX_HANDLE_SIZE +); + +type CFileHandleWrapper = FamStructWrapper; + +#[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, } 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 { 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 { - 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, + ) -> 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::>(); + 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(), + ); + } }