Open shinmao opened 1 year ago
We consider that there are some other misaligned pointer dereference in this package.
First of all, in <linux::vcpu::UhyveCPU as vm::VirtualCPU>::r#continue
:
https://github.com/hermit-os/uhyve/blob/92458f59930789a84effb8aa0b3dc103c35ccd09/src/linux/vcpu.rs#L360-L361
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>()))
.
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
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. :)
@mkroening I would love to help fix the issue in my free time. Feel free to ping me in the future!
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.
@mkroening Hi, I just noticed that the following two methods
could have the totally same misaligned pointer issues as described above. Could you explain more about why the clippy warning would be suppressed here?
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.
Yup! I think it would be easy for you to fix it now 👍
I can't say how difficult it would be. I did not have a close look yet. :)
See https://github.com/hermit-os/uhyve/commit/aa5758ed9e87b516bf794e968967192a336e4331 for the introduction and more such cases.
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.
Can I have a go at this? :)
Can I have a go at this? :)
Sure! Thanks! :)
I have the feeling, that this issue is vacant as of today :sweat_smile:
The source of unsoundness
https://github.com/hermit-os/uhyve/blob/419435763d826e4578d6d234549cce9a37b11e4c/src/linux/virtqueue.rs#L51-L54 Hi, we consider that
ring_elem
is unsound because it allowsu8
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
execute the code and get panic,
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
asbool
types. Run it can get you the results such as[true, true, true, false]
. However, run with miri then you can find thatmem.offset
already access out-of-bound.It means that our
bool
type points to invalid value actually.