Skip to content

Commit

Permalink
unittest: net: Fix undefined behavior in TestHelper::get_default
Browse files Browse the repository at this point in the history 
This function used an unsound `mem::transmute` to invalidly extend a
lifetime with the goal of creating a self-referential struct. Sadly,
after returning from `get_default`, the `GuestMemoryMmap` gets moved to
a different stack frame, and so all references that this function
created become dangling pointers.

Fix this by not trying to create self-referential struct.

Signed-off-by: Patrick Roy <[email protected]>
  • Loading branch information
@roypat
roypat committed Sep 16, 2024
1 parent 271aed8 commit c86a562
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 39 deletions.
79 changes: 53 additions & 26 deletions src/vmm/src/devices/virtio/net/device.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -956,6 +956,7 @@ pub mod tests {
use crate::dumbo::EthernetFrame;
use crate::logger::IncMetric;
use crate::rate_limiter::{BucketUpdate, RateLimiter, TokenBucket, TokenType};
use crate::utilities::test_utils::single_region_mem;
use crate::vstate::memory::{Address, GuestMemory};

#[test]
Expand Down Expand Up @@ -1113,7 +1114,8 @@ pub mod tests {

#[test]
fn test_rx_missing_queue_signal() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();

th.add_desc_chain(NetQueue::Rx, 0, &[(0, 4096, VIRTQ_DESC_F_WRITE)]);
Expand All @@ -1130,7 +1132,8 @@ pub mod tests {

#[test]
fn test_rx_read_only_descriptor() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();

th.add_desc_chain(
Expand All @@ -1150,7 +1153,8 @@ pub mod tests {

#[test]
fn test_rx_short_writable_descriptor() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();

th.add_desc_chain(NetQueue::Rx, 0, &[(0, 100, VIRTQ_DESC_F_WRITE)]);
Expand All @@ -1162,7 +1166,8 @@ pub mod tests {

#[test]
fn test_rx_partial_write() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();

// The descriptor chain is created so that the last descriptor doesn't fit in the
Expand All @@ -1185,7 +1190,8 @@ pub mod tests {

#[test]
fn test_rx_retry() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();

// Add invalid descriptor chain - read only descriptor.
Expand Down Expand Up @@ -1235,7 +1241,8 @@ pub mod tests {

#[test]
fn test_rx_complex_desc_chain() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();

// Create a valid Rx avail descriptor chain with multiple descriptors.
Expand Down Expand Up @@ -1273,7 +1280,8 @@ pub mod tests {

#[test]
fn test_rx_multiple_frames() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();

// Create 2 valid Rx avail descriptor chains. Each one has enough space to fit the
Expand Down Expand Up @@ -1316,7 +1324,8 @@ pub mod tests {

#[test]
fn test_tx_missing_queue_signal() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();
let tap_traffic_simulator = TapTrafficSimulator::new(if_index(&th.net().tap));

Expand All @@ -1336,7 +1345,8 @@ pub mod tests {

#[test]
fn test_tx_writeable_descriptor() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();
let tap_traffic_simulator = TapTrafficSimulator::new(if_index(&th.net().tap));

Expand All @@ -1355,7 +1365,8 @@ pub mod tests {

#[test]
fn test_tx_short_frame() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();
let tap_traffic_simulator = TapTrafficSimulator::new(if_index(&th.net().tap));

Expand All @@ -1377,7 +1388,8 @@ pub mod tests {

#[test]
fn test_tx_big_frame() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();
let tap_traffic_simulator = TapTrafficSimulator::new(if_index(&th.net().tap));

Expand All @@ -1403,7 +1415,8 @@ pub mod tests {

#[test]
fn test_tx_empty_frame() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();
let tap_traffic_simulator = TapTrafficSimulator::new(if_index(&th.net().tap));

Expand All @@ -1425,7 +1438,8 @@ pub mod tests {

#[test]
fn test_tx_retry() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();
let tap_traffic_simulator = TapTrafficSimulator::new(if_index(&th.net().tap));

Expand Down Expand Up @@ -1467,7 +1481,8 @@ pub mod tests {

#[test]
fn test_tx_complex_descriptor() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();
let tap_traffic_simulator = TapTrafficSimulator::new(if_index(&th.net().tap));

Expand Down Expand Up @@ -1495,7 +1510,8 @@ pub mod tests {

#[test]
fn test_tx_tap_failure() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();
// force the next write to the tap to return an error by simply closing the fd
// SAFETY: its a valid fd
Expand All @@ -1519,7 +1535,8 @@ pub mod tests {

#[test]
fn test_tx_multiple_frame() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();
let tap_traffic_simulator = TapTrafficSimulator::new(if_index(&th.net().tap));

Expand Down Expand Up @@ -1672,7 +1689,8 @@ pub mod tests {

#[test]
fn test_process_error_cases() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();

// RX rate limiter events should error since the limiter is not blocked.
Expand All @@ -1697,7 +1715,8 @@ pub mod tests {
// * interrupt_evt.write
#[test]
fn test_read_tap_fail_event_handler() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();
// force the next write to the tap to return an error by simply closing the fd
// SAFETY: its a valid fd
Expand Down Expand Up @@ -1727,7 +1746,8 @@ pub mod tests {

#[test]
fn test_deferred_frame() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();

let rx_packets_count = th.net().metrics.rx_packets_count.count();
Expand Down Expand Up @@ -1779,7 +1799,8 @@ pub mod tests {

#[test]
fn test_rx_rate_limiter_handling() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();

th.net().rx_rate_limiter = RateLimiter::new(0, 0, 0, 0, 0, 0).unwrap();
Expand All @@ -1793,7 +1814,8 @@ pub mod tests {

#[test]
fn test_tx_rate_limiter_handling() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();

th.net().tx_rate_limiter = RateLimiter::new(0, 0, 0, 0, 0, 0).unwrap();
Expand All @@ -1808,7 +1830,8 @@ pub mod tests {

#[test]
fn test_bandwidth_rate_limiter() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();

// Test TX bandwidth rate limiting
Expand Down Expand Up @@ -1948,7 +1971,8 @@ pub mod tests {

#[test]
fn test_ops_rate_limiter() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();

// Test TX ops rate limiting
Expand Down Expand Up @@ -2059,7 +2083,8 @@ pub mod tests {

#[test]
fn test_patch_rate_limiters() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();

th.net().rx_rate_limiter = RateLimiter::new(10, 0, 10, 2, 0, 2).unwrap();
Expand Down Expand Up @@ -2100,7 +2125,8 @@ pub mod tests {

#[test]
fn test_virtio_device() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.activate_net();
let net = th.net.lock().unwrap();

Expand All @@ -2121,7 +2147,8 @@ pub mod tests {
fn test_queues_notification_suppression() {
let features = 1 << VIRTIO_RING_F_EVENT_IDX;

let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);
th.net().set_acked_features(features);
th.activate_net();

Expand Down
6 changes: 4 additions & 2 deletions src/vmm/src/devices/virtio/net/event_handler.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,11 +134,13 @@ impl MutEventSubscriber for Net {
pub mod tests {
use crate::devices::virtio::net::test_utils::test::TestHelper;
use crate::devices::virtio::net::test_utils::NetQueue;
use crate::devices::virtio::net::TX_INDEX;
use crate::devices::virtio::net::{MAX_BUFFER_SIZE, TX_INDEX};
use crate::utilities::test_utils::single_region_mem;

#[test]
fn test_event_handler() {
let mut th = TestHelper::get_default();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);
let mut th = TestHelper::get_default(&mem);

// Push a queue event, use the TX_QUEUE_EVENT in this test.
th.add_desc_chain(NetQueue::Tx, 0, &[(0, 4096, 0)]);
Expand Down
17 changes: 6 additions & 11 deletions src/vmm/src/devices/virtio/net/test_utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ pub mod test {
#![allow(clippy::undocumented_unsafe_blocks)]
use std::os::unix::ffi::OsStrExt;
use std::sync::{Arc, Mutex, MutexGuard};
use std::{cmp, fmt, mem};
use std::{cmp, fmt};

use event_manager::{EventManager, SubscriberId, SubscriberOps};

Expand All @@ -311,18 +311,17 @@ pub mod test {
use crate::devices::virtio::net::test_utils::{
assign_queues, default_net, inject_tap_tx_frame, NetEvent, NetQueue,
};
use crate::devices::virtio::net::{Net, MAX_BUFFER_SIZE, RX_INDEX, TX_INDEX};
use crate::devices::virtio::net::{Net, RX_INDEX, TX_INDEX};
use crate::devices::virtio::queue::{VIRTQ_DESC_F_NEXT, VIRTQ_DESC_F_WRITE};
use crate::devices::virtio::test_utils::{VirtQueue, VirtqDesc};
use crate::logger::IncMetric;
use crate::utilities::test_utils::single_region_mem;
use crate::vstate::memory::{Address, Bytes, GuestAddress, GuestMemoryMmap};

pub struct TestHelper<'a> {
pub event_manager: EventManager<Arc<Mutex<Net>>>,
pub subscriber_id: SubscriberId,
pub net: Arc<Mutex<Net>>,
pub mem: GuestMemoryMmap,
pub mem: &'a GuestMemoryMmap,
pub rxq: VirtQueue<'a>,
pub txq: VirtQueue<'a>,
}
Expand All @@ -343,18 +342,14 @@ pub mod test {
impl<'a> TestHelper<'a> {
const QUEUE_SIZE: u16 = 16;

pub fn get_default() -> TestHelper<'a> {
pub fn get_default(mem: &'a GuestMemoryMmap) -> TestHelper<'a> {
let mut event_manager = EventManager::new().unwrap();
let mut net = default_net();
let mem = single_region_mem(2 * MAX_BUFFER_SIZE);

// transmute mem_ref lifetime to 'a
let mem_ref = unsafe { mem::transmute::<&GuestMemoryMmap, &'a GuestMemoryMmap>(&mem) };

let rxq = VirtQueue::new(GuestAddress(0), mem_ref, Self::QUEUE_SIZE);
let rxq = VirtQueue::new(GuestAddress(0), mem, Self::QUEUE_SIZE);
let txq = VirtQueue::new(
rxq.end().unchecked_align_up(VirtqDesc::ALIGNMENT),
mem_ref,
mem,
Self::QUEUE_SIZE,
);
assign_queues(&mut net, rxq.create_queue(), txq.create_queue());
Expand Down

0 comments on commit c86a562

Please sign in to comment.