bonzini
commented
9 months ago This can be problematic if there is a memory region that starts at GuestAddress(0x0), but the access was not intended to spill into that region. We don't consider memory to be a circular ring buffer which would be the only situation in which we should allow overflowing accesses.
This unfortunately was intentional because a device that does DMA could treat the memory as a circular buffer, if the (buffer, length) pair that is passed from the driver to the device overflows.
In particular, a subtle effect of the patch is that Error::GuestAddressOverflow would change meaning from "the GuestMemory implementation has a bug" to "the guest has a bug". It would not be visible with GuestMemoryMmap, but I am more inclined to treat it as a GuestMemoryMmap bug.
Writing a regression test for the overflowing add is not possible with the current infrastructure because GuestMemoryMmap does not allow creating memory regions with an access that would overflow
This was introduced by:
commit d47c711504016a6322fa00379538b720192e7b2d
Author: Alexandru Agache <aagch@amazon.com>
Date: Mon Jun 10 16:15:02 2019 +0300
GuestRegionMmap: added error check to constructor
The GuestRegionMmap constructor now checks that adding the guest base
address to the length of the mapped region does not overflow.
Signed-off-by: Alexandru Agache <aagch@amazon.com>and it seems to be an unintended consequence of the commit, only caused by the off-by-one addition of mapping.size(). It also allows a zero size, which I think some non-Linux OSes allow zero size mmap). It should be possible to allow 2^64 there:
diff --git a/src/mmap.rs b/src/mmap.rs
index ebd84a0..3434c35 100644
--- a/src/mmap.rs
+++ b/src/mmap.rs
@@ -122,7 +122,8 @@ impl<B> Deref for GuestRegionMmap<B> {
impl<B: Bitmap> GuestRegionMmap<B> {
/// Create a new memory-mapped memory region for the guest's physical memory.
pub fn new(mapping: MmapRegion<B>, guest_base: GuestAddress) -> result::Result<Self, Error> {
- if guest_base.0.checked_add(mapping.size() as u64).is_none() {
+ // mapping.size() == 0 should not happen at all
+ if mapping.size() == 0 || guest_base.0.checking_add(mapping.size() as u64 - 1).is_none() {
return Err(Error::InvalidGuestRegion);
}
diff --git a/src/mmap_unix.rs b/src/mmap_unix.rs
index f01e30b..a2c876b 100644
--- a/src/mmap_unix.rs
+++ b/src/mmap_unix.rs
@@ -163,7 +163,7 @@ impl<B: Bitmap> MmapRegionBuilder<B> {
return Err(Error::Mmap(io::Error::last_os_error()));
}
- #[cfg(miri)]
+ // mmap with zero length is not portable, enforce nonzero size() for MmapRegion
if self.size == 0 {
return Err(Error::Mmap(io::Error::from_raw_os_error(libc::EINVAL)));
}
andreeaflorescu
Summary of the PR
We were allowing overflows if the overflow value would've been 0. This can be problematic if there is a memory region that starts at GuestAddress(0x0), but the access was not intended to spill into that region. We don't consider memory to be a circular ring buffer which would be the only situation in which we should allow overflowing accesses.
While at it, also refactor the checked_add for the toal number of bytes to use
ok_orinstead of a match pattern, as this is easier to read.Writing a regression test for the overflowing add is not possible with the current infrastructure because GuestMemoryMmap does not allow creating memory regions with an access that would overflow (i.e. having a memory region starting at u64::MAX of size 1). Also, we cannot call try_access with a size bigger than the memory region size because we only allow accesses that fit in the memory region.
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 the summary and max 75 characters for each description line.unsafecode is properly documented.