diff --git a/test/syscall_args/test/tests.c b/test/syscall_args/test/tests.c index 03836a9..882d813 100644 --- a/test/syscall_args/test/tests.c +++ b/test/syscall_args/test/tests.c @@ -211,6 +211,32 @@ void test_syscall_args_bufrd_toolarge(void) { TEST_ASSERT_EQUAL(evt.data.err.errcode, UVM32_ERR_MEM_RD); } +void test_syscall_args_bufrd_overflow_bypass(void) { + // run the vm + uvm32_run(&vmst, &evt, 1000); + // check for picktest syscall + TEST_ASSERT_EQUAL(evt.typ, UVM32_EVT_SYSCALL); + TEST_ASSERT_EQUAL(evt.data.syscall.code, SYSCALL_PICKTEST); + uvm32_arg_setval(&vmst, &evt, RET, SYSCALL_C); + + uvm32_run(&vmst, &evt, 1000); + TEST_ASSERT_EQUAL(evt.typ, UVM32_EVT_SYSCALL); + TEST_ASSERT_EQUAL(evt.data.syscall.code, SYSCALL_C); + + // ARG0 is a real, in-bounds pointer supplied by the rom. Simulate bytecode + // supplying an attacker-controlled huge length in ARG1: previously + // (ptrstart + len) wrapped past 2^32 back under the memory limit inside + // get_safeptr(), bypassing the bounds check entirely. + uvm32_arg_setval(&vmst, &evt, ARG1, 0xFFFFFFFF); + + uvm32_slice_t buf = uvm32_arg_getslice(&vmst, &evt, ARG0, ARG1); + TEST_ASSERT_EQUAL(0, buf.len); // must be safely rejected, not silently overflowed + // attempt to run vm, should be stuck in err + uvm32_run(&vmst, &evt, 100); + TEST_ASSERT_EQUAL(evt.typ, UVM32_EVT_ERR); + TEST_ASSERT_EQUAL(evt.data.err.errcode, UVM32_ERR_MEM_RD); +} + void test_syscall_args_read_badram(void) { // run the vm uvm32_run(&vmst, &evt, 1000); diff --git a/uvm32/uvm32.c b/uvm32/uvm32.c index ba4f779..d5e629b 100644 --- a/uvm32/uvm32.c +++ b/uvm32/uvm32.c @@ -190,7 +190,9 @@ static bool get_safeptr(uvm32_state_t *vmst, uint32_t addr, uint32_t len, uvm32_ return false; } else { uint32_t ptrstart = addr - UVM32_EXTRAM_BASE; - if ((ptrstart > vmst->_extramLen) || (ptrstart + len > vmst->_extramLen)) { + // Compare against (limit - ptrstart) rather than (ptrstart + len) to avoid + // len wrapping ptrstart + len back below the limit on unsigned overflow. + if ((ptrstart > vmst->_extramLen) || (len > vmst->_extramLen - ptrstart)) { setStatusErr(vmst, UVM32_ERR_MEM_RD); buf->ptr = (uint8_t *)NULL; buf->len = 0; @@ -202,7 +204,9 @@ static bool get_safeptr(uvm32_state_t *vmst, uint32_t addr, uint32_t len, uvm32_ } } else { uint32_t ptrstart = addr - MINIRV32_RAM_IMAGE_OFFSET; - if ((ptrstart > UVM32_MEMORY_SIZE) || (ptrstart + len > UVM32_MEMORY_SIZE)) { + // Compare against (limit - ptrstart) rather than (ptrstart + len) to avoid + // len wrapping ptrstart + len back below the limit on unsigned overflow. + if ((ptrstart > UVM32_MEMORY_SIZE) || (len > UVM32_MEMORY_SIZE - ptrstart)) { setStatusErr(vmst, UVM32_ERR_MEM_RD); buf->ptr = (uint8_t *)NULL; buf->len = 0;