diff --git a/virtio-devices/src/vsock/device.rs b/virtio-devices/src/vsock/device.rs index 506e2dcba..27a0af1ff 100644 --- a/virtio-devices/src/vsock/device.rs +++ b/virtio-devices/src/vsock/device.rs @@ -130,7 +130,16 @@ where ) { Ok(mut pkt) => { if self.backend.write().unwrap().recv_pkt(&mut pkt).is_ok() { - pkt.hdr().len() as u32 + pkt.len() + match pkt.commit_hdr(&*self.mem.memory()) { + Ok(()) => pkt.hdr().len() as u32 + pkt.len(), + Err(err) => { + warn!( + "vsock: Error writing packet header to guest memory: \ + {err:?}. Discarding the package." + ); + 0 + } + } } else { // We are using a consuming iterator over the virtio buffers, so, if we can't // fill in this buffer, we'll need to undo the last iterator step. diff --git a/virtio-devices/src/vsock/packet.rs b/virtio-devices/src/vsock/packet.rs index a9e477c1d..65d41b06b 100644 --- a/virtio-devices/src/vsock/packet.rs +++ b/virtio-devices/src/vsock/packet.rs @@ -20,7 +20,7 @@ use std::ops::Deref; use byteorder::{ByteOrder, LittleEndian}; use virtio_queue::DescriptorChain; -use vm_memory::{Address, GuestMemory}; +use vm_memory::{Address, Bytes, GuestAddress, GuestMemory}; use vm_virtio::{AccessPlatform, Translatable}; use super::{Result, VsockError, defs}; @@ -96,7 +96,10 @@ const HDROFF_FWD_CNT: usize = 40; /// - (an optional) data/buffer descriptor, only present for data packets (VSOCK_OP_RW). /// pub struct VsockPacket { - hdr: *mut u8, + // We still hold the header address in guest memory. We need to write back the modified + // header in RX buffers. + guest_hdr_addr: GuestAddress, + hdr: [u8; VSOCK_PKT_HDR_SIZE], buf: Option<*mut u8>, buf_size: usize, } @@ -129,14 +132,23 @@ impl VsockPacket { return Err(VsockError::HdrDescTooSmall(head.len())); } + let guest_hdr_addr = head + .addr() + .translate_gva(access_platform, VSOCK_PKT_HDR_SIZE); + + // To avoid TOCTOU issues when reading/writing the VSock packet header in guest memory, + // we need to copy the content of the header in the VMM's memory. + // After the copy, the hdr content can be trusted since the guest can't change its + // content anymore. + let mut hdr = [0u8; VSOCK_PKT_HDR_SIZE]; + desc_chain + .memory() + .read_slice(hdr.as_mut_slice(), guest_hdr_addr) + .map_err(|_| VsockError::GuestMemory)?; + let mut pkt = Self { - hdr: get_host_address_range( - desc_chain.memory(), - head.addr() - .translate_gva(access_platform, VSOCK_PKT_HDR_SIZE), - VSOCK_PKT_HDR_SIZE, - ) - .ok_or(VsockError::GuestMemory)?, + guest_hdr_addr, + hdr, buf: None, buf_size: 0, }; @@ -221,19 +233,28 @@ impl VsockPacket { return Err(VsockError::HdrDescTooSmall(head.len())); } + let guest_hdr_addr = head + .addr() + .translate_gva(access_platform, VSOCK_PKT_HDR_SIZE); + + // To avoid TOCTOU issues when reading/writing the VSock packet header in guest memory, + // we need to copy the content of the header in the VMM's memory. + // After the copy, the hdr content can be trusted since the guest can't change its + // content anymore. + let mut hdr = [0u8; VSOCK_PKT_HDR_SIZE]; + desc_chain + .memory() + .read_slice(hdr.as_mut_slice(), guest_hdr_addr) + .map_err(|_| VsockError::GuestMemory)?; + // Prior to Linux v6.3 there are two descriptors if head.has_next() { let buf_desc = desc_chain.next().ok_or(VsockError::BufDescMissing)?; let buf_size = buf_desc.len() as usize; Ok(Self { - hdr: get_host_address_range( - desc_chain.memory(), - head.addr() - .translate_gva(access_platform, VSOCK_PKT_HDR_SIZE), - VSOCK_PKT_HDR_SIZE, - ) - .ok_or(VsockError::GuestMemory)?, + guest_hdr_addr, + hdr, buf: Some( get_host_address_range( desc_chain.memory(), @@ -247,13 +268,8 @@ impl VsockPacket { } else { let buf_size: usize = head.len() as usize - VSOCK_PKT_HDR_SIZE; Ok(Self { - hdr: get_host_address_range( - desc_chain.memory(), - head.addr() - .translate_gva(access_platform, VSOCK_PKT_HDR_SIZE), - VSOCK_PKT_HDR_SIZE, - ) - .ok_or(VsockError::GuestMemory)?, + guest_hdr_addr, + hdr, buf: Some( get_host_address_range( desc_chain.memory(), @@ -273,17 +289,27 @@ impl VsockPacket { /// Provides in-place, byte-slice, access to the vsock packet header. /// pub fn hdr(&self) -> &[u8] { - // 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) } + self.hdr.as_slice() } /// Provides in-place, byte-slice, mutable access to the vsock packet header. /// pub fn hdr_mut(&mut self) -> &mut [u8] { - // 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) } + self.hdr.as_mut_slice() + } + + /// Writes the local copy of the packet header to the guest memory. + /// + pub fn commit_hdr(&mut self, guest_mem: &M) -> Result<()> { + if self.len() as usize > defs::MAX_PKT_BUF_SIZE { + return Err(VsockError::InvalidPktLen(self.len())); + } + + guest_mem + .write(self.hdr(), self.guest_hdr_addr) + .map_err(|_| VsockError::GuestMemory)?; + + Ok(()) } /// Provides in-place, byte-slice access to the vsock packet data buffer.