rust-vmm / kvm-ioctls

Apache License 2.0
270 stars 106 forks source link

Fix soundness issues via `KvmRunWrapper::as_mut_ref()` #255

Closed 00xc closed 7 months ago

00xc commented 7 months ago

Summary of the PR

As detailed in #248, and since KvmRunWrapper implements Send and Sync, as_mut_ref() can cause undefined behavior, as two threads can acquire a mutable reference to the kvm_run struct via an immutable reference to KvmRunWrapper.

Fix this by making KvmRunWrapper::as_mut_ref() take &mut self, which also gets rid of a clippy warning suppression, and update the callers. This results in potentially breaking changes in the public interface, as several VcpuFd methods now take &mut self as well.

Fixes: #248

Requirements

Before submitting your PR, please make sure you addressed the following requirements:

00xc commented 7 months ago

Ping for the remaining reviewers.

JonathanWoollett-Light commented 7 months ago

This is a breaking change so it would be good to check if anyone has any other concerns, otherwise LGTM. I've messaged in the rust-vmm public slack, if there are no concerns after some time I will approve.

00xc commented 7 months ago

Perhaps there should be a security advisory for this. Producing undefined behavior is pretty simple:

let vcpu = ...;

// Suppose this is a VcpuExit that contains an immutable
// reference to kvm_run, e.g. IoOut, MmioWrite
let exit = vcpu.run().unwrap();

// Suppose this is a VcpuExit of the same type as above.
// This modifies kvm_run again.
vcpu.run().unwrap();

// The value `exit` immutably references has been modified,
// yet the compiler does not complain because `run()` takes
// `&self`. Using `exit` here is UB.
println!("{:?}", exit);

Another way to think about it is that mutable accesses must invalidate all previous immutable references.