Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maybe unsound in virtio's write_ fn groups #855

Open
lwz23 opened this issue Jan 6, 2025 · 1 comment
Open

Maybe unsound in virtio's write_ fn groups #855

lwz23 opened this issue Jan 6, 2025 · 1 comment
Assignees

Comments

@lwz23
Copy link

lwz23 commented Jan 6, 2025

Hello, thank you for your contribution in this project, I am scanning the unsoundness problem in rust project.
I notice the following code:

pub fn write_selected_queue(&mut self, dest: &[u8]) {
		self.selected_queue_num = unsafe {
			#[allow(clippy::cast_ptr_alignment)]
			*(dest.as_ptr() as *const u16)
		};
	}

	// Register virtqueue
	pub fn write_pfn(&mut self, dest: &[u8], mem: &MmapMemory) {
		let status = self.read_status_reg();
		if status & STATUS_FEATURES_OK != 0
			&& status & STATUS_DRIVER_OK == 0
			&& self.selected_queue_num as usize == self.virt_queues.len()
		{
			let gpa = GuestPhysAddr::new(unsafe {
				#[allow(clippy::cast_ptr_alignment)]
				*(dest.as_ptr() as *const u64)
			});
			let hva = mem.host_address(gpa).unwrap();
			let queue = unsafe { Virtqueue::new(hva as *mut u8, QUEUE_LIMIT) };
			self.virt_queues.push(queue);
		}
	}

	pub fn write_requested_features(&mut self, dest: &[u8]) {
		if self.read_status_reg() == STATUS_ACKNOWLEDGE | STATUS_DRIVER {
			let requested_features = unsafe {
				#[allow(clippy::cast_ptr_alignment)]
				*(dest.as_ptr() as *const u32)
			};
			self.requested_features =
				(self.requested_features | requested_features) & HOST_FEATURES;
		}
	}

the problem is dest.as_ptr() as *const u16/32/64 , indices is u8 which is 1-bytes aligned, and convert it to a higher align type may result in unaligned problem which volited to safety precondition of deference a raw pointer. According to rust's security specifications, any pub function that can cause UB should be marked as unsafe, so I decide to report it.

POC

pub struct VirtioNetPciDevice {
	//registers: PciRegisters, //Add more
	requested_features: u32,
	selected_queue_num: u16,
	//virt_queues: Vec<Virtqueue>,
	//iface: Option<Mutex<Iface>>,
	mac_addr: [u8; 6],
}
impl VirtioNetPciDevice {
    pub fn write_selected_queue(&mut self, dest: &[u8]) {
        self.selected_queue_num = unsafe {
            #[allow(clippy::cast_ptr_alignment)]
            *(dest.as_ptr() as *const u16)
        };
    }
    
}
fn main() {
    let mut device = VirtioNetPciDevice {
        requested_features: 0,
        selected_queue_num: 0,
        mac_addr: [0; 6],
    };


    let mut dest = [0u8; 3]; 

    dest[1] = 1;
    dest[2] = 2;

    device.write_selected_queue(&dest); 
}

result:

PS E:\Github\lwz> cargo +nightly miri run
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.00s
     Running `C:\Users\ROG\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\bin\cargo-miri.exe runner target\miri\x86_64-pc-windows-msvc\debug\lwz.exe`
warning: fields `requested_features` and `mac_addr` are never read
 --> src\main.rs:3:2
  |
1 | pub struct VirtioNetPciDevice {
  |            ------------------ fields in this struct
2 |     //registers: PciRegisters, //Add more
3 |     requested_features: u32,
  |     ^^^^^^^^^^^^^^^^^^
...
7 |     mac_addr: [u8; 6],
  |     ^^^^^^^^
  |
  = note: `#[warn(dead_code)]` on by default

error: Undefined Behavior: accessing memory based on pointer with alignment 1, but alignment 2 is required
  --> src\main.rs:13:13
   |
13 |             *(dest.as_ptr() as *const u16)
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ accessing memory based on pointer with alignment 1, but alignment 2 is required
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
   = note: BACKTRACE:
   = note: inside `VirtioNetPciDevice::write_selected_queue` at src\main.rs:13:13: 13:43
note: inside `main`
  --> src\main.rs:31:5
   |
31 |     device.write_selected_queue(&dest);
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error; 1 warning emitted

error: process didn't exit successfully: `C:\Users\ROG\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\bin\cargo-miri.exe runner target\miri\x86_64-pc-windows-msvc\debug\lwz.exe` (exit code: 1)
PS E:\Github\lwz> 
@lwz23
Copy link
Author

lwz23 commented Jan 6, 2025

maybe same problem for

*(self.mem.offset(2) as *const u16)

let write_ptr = self.mem.offset(2) as *mut u16;

unsafe { &mut *(self.mem.offset((4 + index * elem_size) as isize) as *mut T) }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants