`VcpuFd::set_kvm_immediate_exit` may lead to data races/undefined behavior

[alexcrichton]

Currently the VcpuFd structure implements both Send and Sync so it's safe to share across threads, but this method enables mutation of shared memory. The signature for this method takes &self which means it can be invoked concurrently on multiple threads. Its current implementation:

    pub fn set_kvm_immediate_exit(&self, val: u8) {
        let kvm_run = self.kvm_run_ptr.as_mut_ref();
        kvm_run.immediate_exit = val;
    }

currently can expose two issues in Rust's memory model. The first is that the as_mut_ref function cannot be sound because it's promoting a safe shared reference to a safe mutable reference. Mutable references guarantee exclusivitity but that's not the case here with multiple threads. The second issue is that the write here is a non-atomic write as opposed to an atomic write, which means that multiple threads can do non-atomic writes causing a data race.

I found this when poking around this crate (thanks for providing it!). I'm still very new to this space and am trying to get my bearings. I was planning on using this to set this flag from another thread to assist with interrupting a VM, which is why I was curious how it worked internally given its signature.

I think that the fix here is relatively simple, just restructuring things internally. In theory as_mut_ref() should be removed entirely (that method signature can't ever be sound in Rust) and instead code can use ptr::addr_of to convert from *mut kvm_run to *mut u8 for the flag here. That could then be casted to AtomicU8 and the store done through that.

[roypat]

Hi @alexcrichton, thanks for the report! I'm afraid there's a few of these across our crates, we're slowly working towards resolving these. I agree that as_mut_ref is nonsense with its current signature (, at the very least I think it should take self by mutable reference. More generally, I suppose we really should not be taking rust-style references to pointers we get from mmap/KVM at all. We went through that entire conversion in vm-memory in https://github.com/rust-vmm/vm-memory/pull/217 some time ago, seems like we'll need to do a similar thing here

[00xc]

I think that the fix here is relatively simple, just restructuring things internally. In theory as_mut_ref() should be removed entirely (that method signature can't ever be sound in Rust) and instead code can use ptr::addr_of to convert from *mut kvm_run to *mut u8 for the flag here. That could then be casted to AtomicU8 and the store done through that.

Making as_mut_ref() have a &mut self signature would also make it sound, no? It also conveys what the method does better, and gets rid of a clippy warning that is currently being suppressed.

EDIT: just to be clear, this would require two breaking changes to the public API, namely VcpuFd::set_kvm_immediate_exit() and VcpuFd::run() would both now need to take &mut self. In the current state, VcpuFd::sync_regs() would also need to have this change, but it can be avoided by adding a KvmRunWrapper::as_ref() method.