Bug fixes and readability improvements to guest memory handling

libsel4vm/src/guest_memory.c

@@ -69,19 +69,18 @@ struct frames_map_iterator_cookie {
 
 static inline int reservation_node_cmp(res_tree *x, res_tree *y)
 {
-    if (x->addr < y->addr) {
-        if (x->addr + x->size > y->addr) {
-            /* The two regions intersect */
-            return 0;
-        } else {
-            return -1;
-        }
+    if (x->addr < y->addr && y->addr - x->addr >= x->size) {
+        /* x is before y */
+        return -1;
     }
-    if (x->addr < y->addr + y->size) {
-        /* The two regions intersect */
-        return 0;
+
+    if (x->addr > y->addr && x->addr - y->addr >= y->size) {
+        /* y is before x */
+        return 1;
     }
-    return 1;
+
+    /* regions intersect */
+    return 0;
 }
 
 SGLIB_DEFINE_RBTREE_PROTOTYPES(res_tree, left, right, color_field, reservation_node_cmp);
@@ -263,7 +262,7 @@ static vm_memory_reservation_t *find_anon_reservation_by_addr(uintptr_t addr, si
 
     for (int i = 0; i < num_anon_reservations; i++) {
         vm_memory_reservation_t *curr_res = reservations[i];
-        if (curr_res->addr <= addr && curr_res->addr + curr_res->size >= addr + size) {
+        if (is_subregion(curr_res->addr, curr_res->size, addr, size)) {
             return curr_res;
         }
     }
@@ -282,7 +281,7 @@ memory_fault_result_t vm_memory_handle_fault(vm_t *vm, vm_vcpu_t *vcpu, uintptr_
         return FAULT_UNHANDLED;
     }
 
-    if ((reservation_node->addr + size) > (reservation_node->addr + reservation_node->size)) {
+    if (!is_subregion(reservation_node->addr, reservation_node->size, addr, size)) {
         ZF_LOGE("Failed to handle memory fault: Invalid fault region");
         return FAULT_ERROR;
     }
@@ -466,30 +465,42 @@ int vm_free_reserved_memory(vm_t *vm, vm_memory_reservation_t *reservation)
 int map_vm_memory_reservation(vm_t *vm, vm_memory_reservation_t *vm_reservation,
                               memory_map_iterator_fn map_iterator, void *map_cookie)
 {
-    int err;
-    uintptr_t reservation_addr = vm_reservation->addr;
-    size_t reservation_size = vm_reservation->size;
     uintptr_t current_addr = vm_reservation->addr;
+    size_t bytes_left = vm_reservation->size;
 
-    while (current_addr < reservation_addr + reservation_size) {
+    while (bytes_left) {
         vm_frame_t reservation_frame = map_iterator(current_addr, map_cookie);
+
         if (reservation_frame.cptr == seL4_CapNull) {
-            ZF_LOGE("Failed to get frame for reservation address 0x%lx", current_addr);
+            ZF_LOGE("Failed to get frame for reservation address 0x%"PRIxPTR, current_addr);
             break;
         }
+
+        if (bytes_left < BIT(reservation_frame.size_bits)) {
+            ZF_LOGE("Mapping frame of size %zu to 0x%"PRIxPTR "overflows reservation %zu bytes at 0x%"PRIxPTR,
+                    BIT(reservation_frame.size_bits), current_addr,
+                    vm_reservation->size, vm_reservation->addr);
+            break;
+        }
+
         int ret = vspace_deferred_rights_map_pages_at_vaddr(&vm->mem.vm_vspace, &reservation_frame.cptr, NULL,
                                                             (void *)reservation_frame.vaddr, 1, reservation_frame.size_bits,
                                                             reservation_frame.rights, vm_reservation->vspace_reservation);
         if (ret) {
             ZF_LOGE("Failed to map address 0x%"PRIxPTR" into guest vm vspace", reservation_frame.vaddr);
-            return -1;
+            break;
         }
+
+        /* No underflow here: bytes_left >= frame size, see check above */
+        bytes_left -= BIT(reservation_frame.size_bits);
         current_addr += BIT(reservation_frame.size_bits);
     }
+
     vm_reservation->memory_map_iterator = NULL;
     vm_reservation->memory_iterator_cookie = NULL;
-    vm_reservation->is_mapped = true;
-    return 0;
+    vm_reservation->is_mapped = (bytes_left == 0);
+
+    return vm_reservation->is_mapped ? 0 : -1;
 }
 
 int vm_map_reservation(vm_t *vm, vm_memory_reservation_t *reservation,

libsel4vm/src/guest_memory.h

@@ -9,6 +9,21 @@
 #include <sel4vm/guest_vm.h>
 #include <sel4vm/guest_memory.h>
 
+/**
+ * Check whether a region is a subregion of another region
+ * @param {uintptr_t} start           Start of parent region
+ * @param {size_t} size               Size of parent region
+ * @param {uintptr_t} subreg_start    Start of potential subregion
+ * @param {size_t} subreg_size        Size of potential subregion
+ * @return                            True if subregion fits in parent completely, false otherwise
+ */
+static inline bool is_subregion(uintptr_t start, size_t size,
+                                uintptr_t subreg_start, size_t subreg_size)
+{
+    return (subreg_start >= start) && (subreg_size <= size) &&
+           (subreg_start - start <= size - subreg_size);
+}
+
 /**
  * Handle a vm memory fault through searching previously created reservations and invoking the appropriate fault callback
  * @param {vm_t *} vm               A handle to the VM

libsel4vm/src/guest_ram.c

@@ -107,9 +107,10 @@ static bool is_ram_region(vm_t *vm, uintptr_t addr, size_t size)
 {
     vm_mem_t *guest_memory = &vm->mem;
     for (int i = 0; i < guest_memory->num_ram_regions; i++) {
-        if (guest_memory->ram_regions[i].start <= addr &&
-            guest_memory->ram_regions[i].start + guest_memory->ram_regions[i].size >= addr + size) {
-            /* We are within a ram region*/
+        if (is_subregion(guest_memory->ram_regions[i].start,
+                         guest_memory->ram_regions[i].size,
+                         addr, size)) {
+            /* We are within RAM region */
             return true;
         }
     }
@@ -208,8 +209,9 @@ void vm_ram_mark_allocated(vm_t *vm, uintptr_t start, size_t bytes)
     int i;
     int region = -1;
     for (i = 0; i < guest_memory->num_ram_regions; i++) {
-        if (guest_memory->ram_regions[i].start <= start &&
-            guest_memory->ram_regions[i].start + guest_memory->ram_regions[i].size >= start + bytes) {
+        if (is_subregion(guest_memory->ram_regions[i].start,
+                         guest_memory->ram_regions[i].size,
+                         start, bytes)) {
             region = i;
             break;
         }
@@ -299,23 +301,10 @@ static vm_frame_t ram_ut_alloc_iterator(uintptr_t addr, void *cookie)
     return frame_result;
 }
 
-static int map_ram_reservation(vm_t *vm, vm_memory_reservation_t *ram_reservation, bool untyped)
-{
-    int err;
-    /* We map the reservation immediately, by-passing the deferred mapping functionality
-     * This allows us the allocate, touch and manipulate VM RAM prior to the region needing to be
-     * faulted upon first */
-    if (untyped) {
-        err = map_vm_memory_reservation(vm, ram_reservation, ram_ut_alloc_iterator, (void *)vm);
-    } else {
-        err = map_vm_memory_reservation(vm, ram_reservation, ram_alloc_iterator, (void *)vm);
-    }
-    if (err) {
-        ZF_LOGE("Failed to map new ram reservation");
-        return -1;
-    }
-    return 0;
-}
+/* vm_ram_register() family of functions map the reservation immediately,
+ * bypassing the deferred mapping functionality. This allows us to allocate,
+ * touch and manipulate VM RAM before any faults on the region.
+ */
 
 uintptr_t vm_ram_register(vm_t *vm, size_t bytes)
 {
@@ -330,8 +319,9 @@ uintptr_t vm_ram_register(vm_t *vm, size_t bytes)
         ZF_LOGE("Unable to reserve ram region of size 0x%zx", bytes);
         return 0;
     }
-    err = map_ram_reservation(vm, ram_reservation, false);
+    err = map_vm_memory_reservation(vm, ram_reservation, ram_alloc_iterator, vm);
     if (err) {
+        ZF_LOGE("Failed to map reservation %zu bytes at 0x%"PRIxPTR, bytes, base_addr);
         vm_free_reserved_memory(vm, ram_reservation);
         return 0;
     }
@@ -347,27 +337,9 @@ uintptr_t vm_ram_register(vm_t *vm, size_t bytes)
 
 int vm_ram_register_at(vm_t *vm, uintptr_t start, size_t bytes, bool untyped)
 {
-    vm_memory_reservation_t *ram_reservation;
-    int err;
-
-    ram_reservation = vm_reserve_memory_at(vm, start, bytes, default_ram_fault_callback,
-                                           NULL);
-    if (!ram_reservation) {
-        ZF_LOGE("Unable to reserve ram region at addr 0x%"PRIxPTR" of size 0x%zx", start, bytes);
-        return -1;
-    }
-    err = map_ram_reservation(vm, ram_reservation, untyped);
-    if (err) {
-        vm_free_reserved_memory(vm, ram_reservation);
-        return -1;
-    }
-    err = expand_guest_ram_region(vm, start, bytes);
-    if (err) {
-        ZF_LOGE("Failed to register new ram region");
-        vm_free_reserved_memory(vm, ram_reservation);
-        return -1;
-    }
-    return 0;
+    return vm_ram_register_at_custom_iterator(vm, start, bytes,
+                                              untyped ? ram_ut_alloc_iterator : ram_alloc_iterator,
+                                              vm);
 }
 
 int vm_ram_register_at_custom_iterator(vm_t *vm, uintptr_t start, size_t bytes, memory_map_iterator_fn map_iterator,
@@ -384,7 +356,7 @@ int vm_ram_register_at_custom_iterator(vm_t *vm, uintptr_t start, size_t bytes,
     }
     err = map_vm_memory_reservation(vm, ram_reservation, map_iterator, cookie);
     if (err) {
-        ZF_LOGE("failed to map vm memory reservation to dataport\n");
+        ZF_LOGE("Failed to map reservation %zu bytes at 0x%"PRIxPTR, bytes, start);
         return -1;
     }
     err = expand_guest_ram_region(vm, start, bytes);