-
Notifications
You must be signed in to change notification settings - Fork 106
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
Fix soundness issues via KvmRunWrapper::as_mut_ref()
#255
Conversation
Introduce a new method to get an immutable reference to the kvm_run struct. Replace uses of `as_mut_ref()` with `as_ref()` where possible Signed-off-by: Carlos López <[email protected]>
8a8d8fa
to
503a1d1
Compare
Since KvmRunWrapper implements Send and Sync, this method 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: rust-vmm#248 Signed-off-by: Carlos López <[email protected]>
503a1d1
to
d3a313b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this :)
Ping for the remaining reviewers. |
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. |
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. |
Summary of the PR
As detailed in #248, and since
KvmRunWrapper
implementsSend
andSync
,as_mut_ref()
can cause undefined behavior, as two threads can acquire a mutable reference to thekvm_run
struct via an immutable reference toKvmRunWrapper
.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 severalVcpuFd
methods now take&mut self
as well.Fixes: #248
Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s
), and the commit message has max 60 characters for thesummary and max 75 characters for each description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafe
code is properly documented.