Unsoundness in the implementation of several functions

[shinmao]

The source of unsoundness

uhyve/src/linux/virtqueue.rs

Lines 51 to 54 in 4194357

	pub fn ring_elem(&mut self, index: u16) -> &mut T {
	let elem_size = mem::size_of::<T>() as u16;
	unsafe { &mut *(self.mem.offset((4 + index * elem_size) as isize) as *mut T) }
	}

Hi, we consider that ring_elem is unsound because it allows u8 pointer cast to arbitrary pointer types and dereference the types. It can break the alignment and the validity invariants at the same time.

To reproduce the bug

To reproduce the misalignment issue

use uhyvelib::linux::virtqueue::Vring;

fn main() {
    let mem: u8 = 0;
    let mut vr = Vring::new(&mem as *const u8);
    let em: &mut [u16; 4] = vr.ring_elem(0u16);
    println!("{:?}", em);
}

execute the code and get panic,

thread 'main' panicked at 'misaligned pointer dereference: address must be a multiple of 0x2 but is 0x7fff8b2c183b', /${HOME}/.cargo/registry/src/index.crates.io-6f17d22bba15001f/uhyve-0.2.2/src/linux/virtqueue.rs:53:18

The misaligned pointer dereference is triggered here because you cast the type aligned to 1 byte to the one aligned to 2 bytes. If this is what you plan, then you could change dereference to read_unaligned.

Here, we can also specify em as bool types. Run it can get you the results such as [true, true, true, false]. However, run with miri then you can find that mem.offset already access out-of-bound.

error: Undefined Behavior: out-of-bounds pointer arithmetic: alloc1424 has size 1, so pointer to 4 bytes starting at offset 0 is out-of-bounds
   --> /${HOME}/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/const_ptr.rs:466:18
    |
466 |         unsafe { intrinsics::offset(self, count) }
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds pointer arithmetic: alloc1424 has size 1, so pointer to 4 bytes starting at offset 0 is out-of-bounds

It means that our bool type points to invalid value actually.

[shinmao]

We consider that there are some other misaligned pointer dereference in this package.

The source of unsoundness

First of all, in <linux::vcpu::UhyveCPU as vm::VirtualCPU>::r#continue:

uhyve/src/linux/vcpu.rs

Lines 360 to 361 in 92458f5

	VcpuExit::IoOut(port, addr) => {
	let data_addr: usize = unsafe { (*(addr.as_ptr() as *const u32)) as usize };

addr was cast to u32 and cause to misaligned pointer dereference!

Secondly, the methods implementated for uhyvelib::linux::virtqueue::Vring are unsound. The unsound methods include _flags, index, and advance_index. Worth noting that the usage of offset in index and advance_index cannot avoid alignment issues. It should be changed to the code such as self.mem.add(self.mem.align_offset(align_of::<u16>())).

To reproduce the bug

use uhyvelib::linux::virtqueue::Vring;

fn main() {
    let a: u8 = 1;
    let v = Vring::<u8>::new(&a as *const u8);
    // println!("{}", v._flags());
    println!("{}", v.index());
}

This code shows how to trigger undefined behavior with _flags and index. Just run the code,

thread 'main' panicked at /${HOME}/.cargo/registry/src/index.crates.io-6f17d22bba15001f/uhyve-0.2.2/src/linux/virtqueue.rs:38:13:
misaligned pointer dereference: address must be a multiple of 0x2 but is 0x7fff6343e199

[mkroening]

Thank you so much for opening this issue. We are aware that our virtio implementation is problematic, both in the kernel and in Uhyve.

We are making plans on addressing those problems in the long term. If you would like to contribute, please tell us, so we can coordinate.

It would be helpful if this issue could be split up into two separate issues. The VcpuExit::IoOut(port, addr) might be solved separately. :)

[shinmao]

@mkroening I would love to help fix the issue in my free time. Feel free to ping me in the future!

[mkroening]

Sounds great! Contributions are always welcome. We have a Zulip for chatting and coordination. :)

I have also just added links to Zulip to the organizations README for better discoverability.

[shinmao]

@mkroening Hi, I just noticed that the following two methods

https://github.com/hermit-os/uhyve/blob/fcdd579ae2260339d8111d64e77914f52da940ea/src/linux/virtio.rs#L258_L261

uhyve/src/linux/virtio.rs

Lines 282 to 286 in fcdd579

	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)

could have the totally same misaligned pointer issues as described above. Could you explain more about why the clippy warning would be suppressed here?

[mkroening]

Good find! That should definitely be fixed. I can only guess that this was just to silence clippy without being aware of the consequences and opening an issue.

[shinmao]

Yup! I think it would be easy for you to fix it now 👍

[mkroening]

I can't say how difficult it would be. I did not have a close look yet. :)

See aa5758e for the introduction and more such cases.

[shinmao]

Sorry I can't say that.

Personally speaking, I suggest to use read_unaligned rather than just dereference. It might be able to patch the code with minimum budget.

[jmintb]

Can I have a go at this? :)

[mkroening]

Can I have a go at this? :)

Sure! Thanks! :)

[jounathaen]

I have the feeling, that this issue is vacant as of today 😅