try_access: use checked_add instead of overflowing_add

[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_or instead 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:

• All commits in this PR are signed (with git commit -s), and the commit
  message has max 60 characters for the summary and max 75 characters for each
  description line.
• All added/changed functionality has a corresponding unit/integration
  test.
• All added/changed public-facing functionality has entries in the "Upcoming
  Release" section of CHANGELOG.md (if no such section exists, please create one).
• Any newly added unsafe code is properly documented.

[bonzini]

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 <[email protected]>
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 <[email protected]>

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]

Hmm, ok that's an interesting part of the history (unfortunately an undocumented one). Thanks for the clarification @bonzini . The problem is that we don't have any test to check that this works, and it didn't work before the patch either. If you have a region at the end of u64::MAX, and another one that starts at GuestAddress(0), then try_access does not do the right thing because it will always try to read maximum last region bytes (at least this is what it does in the test I am adding with this patch; this had the same behaviour before my changes as well). I haven't investigated why this is the case.

As a next step in this case we should:

• document that try_access works as a ring buffer and why is that the case
• fix it so that it works as expected

[andreeaflorescu]

Oh, and add a test to validate that this works now...

[bonzini]

Ok, challenge accepted. :) Give me a couple days.