From c45d24df16129fab68b8c92be7fba6432ad800dc Mon Sep 17 00:00:00 2001 From: Wei Liu Date: Wed, 16 Nov 2022 22:42:25 +0000 Subject: [PATCH] virtio-devices: modify or provide safety comments Signed-off-by: Wei Liu --- virtio-devices/src/balloon.rs | 3 ++- virtio-devices/src/console.rs | 1 + virtio-devices/src/epoll_helper.rs | 1 + virtio-devices/src/iommu.rs | 14 +++++++++++++- virtio-devices/src/mem.rs | 2 ++ virtio-devices/src/transport/pci_device.rs | 4 ++-- virtio-devices/src/vhost_user/fs.rs | 6 ++++++ virtio-devices/src/vhost_user/mod.rs | 1 + virtio-devices/src/vhost_user/vu_common_ctrl.rs | 5 ++++- virtio-devices/src/vsock/packet.rs | 9 +++++---- virtio-devices/src/vsock/unix/muxer.rs | 1 + virtio-devices/src/watchdog.rs | 3 +++ 12 files changed, 41 insertions(+), 9 deletions(-) diff --git a/virtio-devices/src/balloon.rs b/virtio-devices/src/balloon.rs index eb0823f62..fb7762de0 100644 --- a/virtio-devices/src/balloon.rs +++ b/virtio-devices/src/balloon.rs @@ -128,8 +128,8 @@ impl BalloonEpollHandler { let hva = memory .get_host_address(range_base) .map_err(Error::GuestMemory)?; - // Need unsafe to do syscall madvise let res = + // SAFETY: Need unsafe to do syscall madvise unsafe { libc::madvise(hva as *mut libc::c_void, range_len as libc::size_t, advice) }; if res != 0 { return Err(Error::MadviseFail(io::Error::last_os_error())); @@ -147,6 +147,7 @@ impl BalloonEpollHandler { ))?; if let Some(f_off) = region.file_offset() { let offset = range_base.0 - region.start_addr().0; + // SAFETY: FFI call with valid arguments let res = unsafe { libc::fallocate64( f_off.file().as_raw_fd(), diff --git a/virtio-devices/src/console.rs b/virtio-devices/src/console.rs index 378f3bbc8..c3a9f341e 100644 --- a/virtio-devices/src/console.rs +++ b/virtio-devices/src/console.rs @@ -607,6 +607,7 @@ fn get_win_size(tty: &dyn AsRawFd) -> (u16, u16) { } let ws: WindowSize = WindowSize::default(); + // SAFETY: FFI call with correct arguments unsafe { libc::ioctl(tty.as_raw_fd(), TIOCGWINSZ, &ws); } diff --git a/virtio-devices/src/epoll_helper.rs b/virtio-devices/src/epoll_helper.rs index 7be1f6407..0de375a3c 100644 --- a/virtio-devices/src/epoll_helper.rs +++ b/virtio-devices/src/epoll_helper.rs @@ -87,6 +87,7 @@ impl EpollHelper { // Create the epoll file descriptor let epoll_fd = epoll::create(true).map_err(EpollHelperError::CreateFd)?; // Use 'File' to enforce closing on 'epoll_fd' + // SAFETY: epoll_fd is a valid fd let epoll_file = unsafe { File::from_raw_fd(epoll_fd) }; let mut helper = Self { diff --git a/virtio-devices/src/iommu.rs b/virtio-devices/src/iommu.rs index 3dbbc1dbb..22cda055b 100644 --- a/virtio-devices/src/iommu.rs +++ b/virtio-devices/src/iommu.rs @@ -267,19 +267,31 @@ struct VirtioIommuFault { address: u64, } -// SAFETY: these data structures only contain integers and have no implicit padding +// SAFETY: data structure only contain integers and have no implicit padding unsafe impl ByteValued for VirtioIommuRange32 {} +// SAFETY: data structure only contain integers and have no implicit padding unsafe impl ByteValued for VirtioIommuRange64 {} +// SAFETY: data structure only contain integers and have no implicit padding unsafe impl ByteValued for VirtioIommuConfig {} +// SAFETY: data structure only contain integers and have no implicit padding unsafe impl ByteValued for VirtioIommuReqHead {} +// SAFETY: data structure only contain integers and have no implicit padding unsafe impl ByteValued for VirtioIommuReqTail {} +// SAFETY: data structure only contain integers and have no implicit padding unsafe impl ByteValued for VirtioIommuReqAttach {} +// SAFETY: data structure only contain integers and have no implicit padding unsafe impl ByteValued for VirtioIommuReqDetach {} +// SAFETY: data structure only contain integers and have no implicit padding unsafe impl ByteValued for VirtioIommuReqMap {} +// SAFETY: data structure only contain integers and have no implicit padding unsafe impl ByteValued for VirtioIommuReqUnmap {} +// SAFETY: data structure only contain integers and have no implicit padding unsafe impl ByteValued for VirtioIommuReqProbe {} +// SAFETY: data structure only contain integers and have no implicit padding unsafe impl ByteValued for VirtioIommuProbeProperty {} +// SAFETY: data structure only contain integers and have no implicit padding unsafe impl ByteValued for VirtioIommuProbeResvMem {} +// SAFETY: data structure only contain integers and have no implicit padding unsafe impl ByteValued for VirtioIommuFault {} #[derive(Error, Debug)] diff --git a/virtio-devices/src/mem.rs b/virtio-devices/src/mem.rs index b8df227a0..d18c95553 100644 --- a/virtio-devices/src/mem.rs +++ b/virtio-devices/src/mem.rs @@ -417,6 +417,7 @@ impl MemEpollHandler { fn discard_memory_range(&self, offset: u64, size: u64) -> Result<(), Error> { // Use fallocate if the memory region is backed by a file. if let Some(fd) = self.host_fd { + // SAFETY: FFI call with valid arguments let res = unsafe { libc::fallocate64( fd, @@ -435,6 +436,7 @@ impl MemEpollHandler { // Only use madvise if the memory region is not allocated with // hugepages. if !self.hugepages { + // SAFETY: FFI call with valid arguments let res = unsafe { libc::madvise( (self.host_addr + offset) as *mut libc::c_void, diff --git a/virtio-devices/src/transport/pci_device.rs b/virtio-devices/src/transport/pci_device.rs index 7654930ca..c08019c4e 100644 --- a/virtio-devices/src/transport/pci_device.rs +++ b/virtio-devices/src/transport/pci_device.rs @@ -666,8 +666,8 @@ impl VirtioPciDevice { .unwrap(); } } else { - // Safe since we know self.cap_pci_cfg_info.cap.cap.offset is 32bits long. let bar_offset: u32 = + // SAFETY: we know self.cap_pci_cfg_info.cap.cap.offset is 32bits long. unsafe { std::mem::transmute(self.cap_pci_cfg_info.cap.cap.offset) }; self.read_bar(0, bar_offset as u64, data) } @@ -687,8 +687,8 @@ impl VirtioPciDevice { right[..data_len].copy_from_slice(data); None } else { - // Safe since we know self.cap_pci_cfg_info.cap.cap.offset is 32bits long. let bar_offset: u32 = + // SAFETY: we know self.cap_pci_cfg_info.cap.cap.offset is 32bits long. unsafe { std::mem::transmute(self.cap_pci_cfg_info.cap.cap.offset) }; self.write_bar(0, bar_offset as u64, data) } diff --git a/virtio-devices/src/vhost_user/fs.rs b/virtio-devices/src/vhost_user/fs.rs index b2a07d04a..6cc7159c2 100644 --- a/virtio-devices/src/vhost_user/fs.rs +++ b/virtio-devices/src/vhost_user/fs.rs @@ -95,6 +95,7 @@ impl VhostUserMasterReqHandler for SlaveReqHandler { let addr = self.mmap_cache_addr + offset; let flags = fs.flags[i]; + // SAFETY: FFI call with valid arguments let ret = unsafe { libc::mmap( addr as *mut libc::c_void, @@ -136,6 +137,7 @@ impl VhostUserMasterReqHandler for SlaveReqHandler { } let addr = self.mmap_cache_addr + offset; + // SAFETY: FFI call with valid arguments let ret = unsafe { libc::mmap( addr as *mut libc::c_void, @@ -172,6 +174,7 @@ impl VhostUserMasterReqHandler for SlaveReqHandler { let addr = self.mmap_cache_addr + offset; let ret = + // SAFETY: FFI call with valid arguments unsafe { libc::msync(addr as *mut libc::c_void, len as usize, libc::MS_SYNC) }; if ret == -1 { return Err(io::Error::last_os_error()); @@ -228,6 +231,7 @@ impl VhostUserMasterReqHandler for SlaveReqHandler { == VhostUserFSSlaveMsgFlags::MAP_W { debug!("write: foffset={}, len={}", foffset, len); + // SAFETY: FFI call with valid arguments unsafe { pwrite64( fd.as_raw_fd(), @@ -238,6 +242,7 @@ impl VhostUserMasterReqHandler for SlaveReqHandler { } } else { debug!("read: foffset={}, len={}", foffset, len); + // SAFETY: FFI call with valid arguments unsafe { pread64(fd.as_raw_fd(), ptr as *mut c_void, len, foffset as off64_t) } }; @@ -279,6 +284,7 @@ impl Default for VirtioFsConfig { } } +// SAFETY: only a series of integers unsafe impl ByteValued for VirtioFsConfig {} pub struct Fs { diff --git a/virtio-devices/src/vhost_user/mod.rs b/virtio-devices/src/vhost_user/mod.rs index fab3fe8c9..9f242fa3e 100644 --- a/virtio-devices/src/vhost_user/mod.rs +++ b/virtio-devices/src/vhost_user/mod.rs @@ -375,6 +375,7 @@ impl VhostUserCommon { pub fn shutdown(&mut self) { if let Some(vu) = &self.vu { + // SAFETY: trivially safe let _ = unsafe { libc::close(vu.lock().unwrap().socket_handle().as_raw_fd()) }; } diff --git a/virtio-devices/src/vhost_user/vu_common_ctrl.rs b/virtio-devices/src/vhost_user/vu_common_ctrl.rs index b077a5817..7bbf936ce 100644 --- a/virtio-devices/src/vhost_user/vu_common_ctrl.rs +++ b/virtio-devices/src/vhost_user/vu_common_ctrl.rs @@ -451,7 +451,7 @@ impl VhostUserHandle { ) .map_err(Error::MemfdCreate)?; - // Safe because we checked the file descriptor is valid + // SAFETY: we checked the file descriptor is valid let file = unsafe { File::from_raw_fd(fd) }; // The size of the memory mapping corresponds to the size of a bitmap // covering all guest pages for addresses from 0 to the last physical @@ -465,6 +465,7 @@ impl VhostUserHandle { file.set_len(mmap_size).map_err(Error::SetFileSize)?; // Set the seals + // SAFETY: FFI call with valid arguments let res = unsafe { libc::fcntl( file.as_raw_fd(), @@ -569,6 +570,7 @@ impl VhostUserHandle { // Be careful with the size, as it was based on u8, meaning we must // divide it by 8. let len = region.size() / 8; + // SAFETY: region is of size len let bitmap = unsafe { // Cast the pointer to u64 let ptr = region.as_ptr() as *const u64; @@ -582,6 +584,7 @@ impl VhostUserHandle { } fn memfd_create(name: &ffi::CStr, flags: u32) -> std::result::Result { + // SAFETY: FFI call with valid arguments let res = unsafe { libc::syscall(libc::SYS_memfd_create, name.as_ptr(), flags) }; if res < 0 { diff --git a/virtio-devices/src/vsock/packet.rs b/virtio-devices/src/vsock/packet.rs index 0c20c4860..1993b9129 100644 --- a/virtio-devices/src/vsock/packet.rs +++ b/virtio-devices/src/vsock/packet.rs @@ -239,7 +239,7 @@ impl VsockPacket { /// Provides in-place, byte-slice, access to the vsock packet header. /// pub fn hdr(&self) -> &[u8] { - // This is safe since bound checks have already been performed when creating the packet + // SAFETY: bound checks have already been performed when creating the packet // from the virtq descriptor. unsafe { std::slice::from_raw_parts(self.hdr as *const u8, VSOCK_PKT_HDR_SIZE) } } @@ -247,7 +247,7 @@ impl VsockPacket { /// Provides in-place, byte-slice, mutable access to the vsock packet header. /// pub fn hdr_mut(&mut self) -> &mut [u8] { - // This is safe since bound checks have already been performed when creating the packet + // SAFETY: bound checks have already been performed when creating the packet // from the virtq descriptor. unsafe { std::slice::from_raw_parts_mut(self.hdr, VSOCK_PKT_HDR_SIZE) } } @@ -261,7 +261,7 @@ impl VsockPacket { /// is stored in the packet header, and accessible via `VsockPacket::len()`. pub fn buf(&self) -> Option<&[u8]> { self.buf.map(|ptr| { - // This is safe since bound checks have already been performed when creating the packet + // SAFETY: bound checks have already been performed when creating the packet // from the virtq descriptor. unsafe { std::slice::from_raw_parts(ptr as *const u8, self.buf_size) } }) @@ -276,7 +276,7 @@ impl VsockPacket { /// is stored in the packet header, and accessible via `VsockPacket::len()`. pub fn buf_mut(&mut self) -> Option<&mut [u8]> { self.buf.map(|ptr| { - // This is safe since bound checks have already been performed when creating the packet + // SAFETY: bound checks have already been performed when creating the packet // from the virtq descriptor. unsafe { std::slice::from_raw_parts_mut(ptr, self.buf_size) } }) @@ -383,6 +383,7 @@ impl VsockPacket { } #[cfg(test)] +#[allow(clippy::undocumented_unsafe_blocks)] mod tests { use super::super::tests::TestContext; use super::*; diff --git a/virtio-devices/src/vsock/unix/muxer.rs b/virtio-devices/src/vsock/unix/muxer.rs index 1cfd7e256..20922db6e 100644 --- a/virtio-devices/src/vsock/unix/muxer.rs +++ b/virtio-devices/src/vsock/unix/muxer.rs @@ -334,6 +334,7 @@ impl VsockMuxer { // device activation time. let epoll_fd = epoll::create(true).map_err(Error::EpollFdCreate)?; // Use 'File' to enforce closing on 'epoll_fd' + // SAFETY: epoll_fd is a valid fd let epoll_file = unsafe { File::from_raw_fd(epoll_fd) }; // Open/bind/listen on the host Unix socket, so we can accept host-initiated diff --git a/virtio-devices/src/watchdog.rs b/virtio-devices/src/watchdog.rs index 105a8f788..7062225a1 100644 --- a/virtio-devices/src/watchdog.rs +++ b/virtio-devices/src/watchdog.rs @@ -235,6 +235,7 @@ impl Watchdog { error!("Failed to create timer fd {}", e); e })?; + // SAFETY: timer_fd is a valid fd let timer = unsafe { File::from_raw_fd(timer_fd) }; Ok(Watchdog { @@ -280,6 +281,7 @@ impl Drop for Watchdog { } fn timerfd_create() -> Result { + // SAFETY: FFI call, trivially safe let res = unsafe { libc::timerfd_create(libc::CLOCK_MONOTONIC, 0) }; if res < 0 { Err(io::Error::last_os_error()) @@ -301,6 +303,7 @@ fn timerfd_setup(timer: &File, secs: i64) -> Result<(), io::Error> { }; let res = + // SAFETY: FFI call with correct arguments unsafe { libc::timerfd_settime(timer.as_raw_fd(), 0, &periodic, std::ptr::null_mut()) }; if res < 0 {