From 4d1ebf25e0e5d83e14db6996a8484aaa8436557d Mon Sep 17 00:00:00 2001 From: Hannu Lyytinen Date: Wed, 24 May 2023 10:31:21 +0300 Subject: [PATCH 1/4] libsel4vm: Fix reservation mapping Comment in vm_map_reservation() about resetting the iterator was only partially true. Return value did not report any errors either. Also avoid integer overflows and fix a format string. Signed-off-by: Hannu Lyytinen --- libsel4vm/src/guest_memory.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/libsel4vm/src/guest_memory.c b/libsel4vm/src/guest_memory.c index 48e5be172..43b533b8c 100644 --- a/libsel4vm/src/guest_memory.c +++ b/libsel4vm/src/guest_memory.c @@ -466,30 +466,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, From f18f0a9ea20e69bc785bfc2e3e3b0adf2a5f7247 Mon Sep 17 00:00:00 2001 From: Hannu Lyytinen Date: Fri, 19 May 2023 13:15:34 +0300 Subject: [PATCH 2/4] libsel4vm: Remove duplicate code Signed-off-by: Hannu Lyytinen --- libsel4vm/src/guest_ram.c | 50 ++++++++------------------------------- 1 file changed, 10 insertions(+), 40 deletions(-) diff --git a/libsel4vm/src/guest_ram.c b/libsel4vm/src/guest_ram.c index 5a10b86ba..6fb0ab04f 100644 --- a/libsel4vm/src/guest_ram.c +++ b/libsel4vm/src/guest_ram.c @@ -299,23 +299,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 +317,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 +335,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 +354,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); From a32e5d6bea76828ac24467dc3e75b2b84dd47c06 Mon Sep 17 00:00:00 2001 From: Hannu Lyytinen Date: Fri, 19 May 2023 12:17:53 +0300 Subject: [PATCH 3/4] libsel4vm: Check subregions correctly LH side of comparison in vm_memory_handle_fault() uses wrong value. Avoid potential overflows in other places as well. Signed-off-by: Hannu Lyytinen --- libsel4vm/src/guest_memory.c | 4 ++-- libsel4vm/src/guest_memory.h | 15 +++++++++++++++ libsel4vm/src/guest_ram.c | 12 +++++++----- 3 files changed, 24 insertions(+), 7 deletions(-) diff --git a/libsel4vm/src/guest_memory.c b/libsel4vm/src/guest_memory.c index 43b533b8c..85f24d8ed 100644 --- a/libsel4vm/src/guest_memory.c +++ b/libsel4vm/src/guest_memory.c @@ -263,7 +263,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 +282,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; } diff --git a/libsel4vm/src/guest_memory.h b/libsel4vm/src/guest_memory.h index 28e55037f..945443543 100644 --- a/libsel4vm/src/guest_memory.h +++ b/libsel4vm/src/guest_memory.h @@ -9,6 +9,21 @@ #include #include +/** + * 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 diff --git a/libsel4vm/src/guest_ram.c b/libsel4vm/src/guest_ram.c index 6fb0ab04f..3ae71f6d0 100644 --- a/libsel4vm/src/guest_ram.c +++ b/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; } From f58d0541c687427c0fbc74aa234036773e6ca969 Mon Sep 17 00:00:00 2001 From: Hannu Lyytinen Date: Fri, 19 May 2023 12:18:08 +0300 Subject: [PATCH 4/4] libsel4vm: Avoid overflowing arithmetic Signed-off-by: Hannu Lyytinen --- libsel4vm/src/guest_memory.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/libsel4vm/src/guest_memory.c b/libsel4vm/src/guest_memory.c index 85f24d8ed..2b91b1f47 100644 --- a/libsel4vm/src/guest_memory.c +++ b/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);