Add missing stack check. The ABI has included space for the required stack space from the start, but we only recently started putting anything sensible there. It turns out that the switcher was missing this check. This introduces a new errno code to let the caller differentiate between a call that wasn't allowed to proceed and one that crashed. We may want to use this code for other stack badness.
diff --git a/sdk/core/switcher/entry.S b/sdk/core/switcher/entry.S index 13b6523..8836d08 100644 --- a/sdk/core/switcher/entry.S +++ b/sdk/core/switcher/entry.S
@@ -3,6 +3,7 @@ #include "export-table-assembly.h" #include "trusted-stack-assembly.h" +#include <errno.h> .include "assembly-helpers.s" @@ -246,6 +247,17 @@ // table pointer. Nothing between this point and transition to the callee // should fault. csc ct1, TrustedStackFrame_offset_calleeExportTable(ctp) + + // Load the minimum stack size required by the callee. + clbu tp, ExportEntry_offset_minimumStackSize(ct1) + // The stack size is in 8-byte units, so multiply by 8. + slli tp, tp, 3 + // Check that the stack is large enough for the callee. + // At this point, we have already truncated the stack and so the length of + // the stack is the length that the callee can use. + cgetlen t2, csp + bgtu tp, t2, .Lstack_too_small + // Get the flags field into tp clbu tp, ExportEntry_offset_flags(ct1) cgetbase s1, ct1 @@ -289,6 +301,7 @@ zeroRegisters tp, t1, t2, s0, s1 cjalr cra +.Lskip_compartment_call: // If we are doing a forced unwind of the trusted stack then we do almost // exactly the same as a normal unwind. We will jump here from the // exception path. @@ -302,6 +315,15 @@ // ca1, used for second return value zeroAllRegistersExcept ra, sp, gp, s0, s1, a0, a1 cret + + // If the stack is too small, we don't do the call, but to avoid leaking + // any other state we still go through the same return path as normal. We + // set the return registers to -ENOTENOUGHSTACK and 0, so users can see + // that this is the failure reason. +.Lstack_too_small: + li a0, -ENOTENOUGHSTACK + li a1, 0 + j .Lskip_compartment_call .size compartment_switcher_entry, . - compartment_switcher_entry // the entry point of all exceptions and interrupts
diff --git a/sdk/include/errno.h b/sdk/include/errno.h index 1dadd3c..df74785 100644 --- a/sdk/include/errno.h +++ b/sdk/include/errno.h
@@ -84,6 +84,7 @@ #define EOWNERDEAD 130 // Previous owner died. #define ENOTRECOVERABLE 131 // State not recoverable. #define EOVERFLOW 139 // Value too large to be stored in data type. +#define ENOTENOUGHSTACK 140 // Insufficient stack space for cross-compartment call. #define EWOULDBLOCK EAGAIN // Operation would block. #define ENOTSUP EOPNOTSUPP // Not supported. #define __ELASTERROR 2000 // Users can add values starting here.
diff --git a/tests/stack-test.cc b/tests/stack-test.cc index b51ea6b..71ebd27 100644 --- a/tests/stack-test.cc +++ b/tests/stack-test.cc
@@ -72,8 +72,45 @@ debug_log("Expected to invoke the handler? {}", handlerExpected); set_expected_behaviour(&threadStackTestFailed, handlerExpected); } + + __attribute__((used)) extern "C" int test_small_stack() + { + return test_stack_requirement(); + } } // namespace +__attribute__((used)) extern "C" int test_with_small_stack(size_t stackSize); + +asm(".section .text\n" + ".global test_with_small_stack\n" + "test_with_small_stack:\n" + // Preserve the old stack + " cmove ct0, csp\n" + // Add space to the requested size for the spill slots and the size of the + // stack that `test_small_stack` will use, plus the four capabilities that + // the switcher will spill for us. We use 16 bytes to store the stack + // pointer and return address, `test_small_stack` uses the same amount: it + // needs to store the return address, and the ABI requires that stacks are + // 16-byte aligned and so it uses a full 16 bytes. + " add a0, a0, 64\n" + // Get the base of the stack + " cgetbase a2, csp\n" + // Move the stack pointer to the current base + " csetaddr csp, csp, a2\n" + // Truncate the stack + " csetbounds csp, csp, a0\n" + // Move to the end of the stack, minus the spill-slot size + " cincoffset csp, csp, a0\n" + " cincoffset csp, csp, -16\n" + " csc ct0, 0(csp)\n" + " csc cra, 8(csp)\n" + // Call the test function + " cjal test_small_stack\n" + // Restore + " clc cra, 8(csp)\n" + " clc csp, 0(csp)\n" + " cjr cra\n"); + // Defeat the compiler optimisation that may turn our first call to this into a // call. If the compiler does this then we will fail on an even number of // cross-compartment calls not an odd number. @@ -90,6 +127,18 @@ */ void test_stack() { + int ret = test_with_small_stack(128); + TEST(ret == 0, + "test_with_small_stack failed, returned {} with 128-byte stack", + ret); + ret = test_with_small_stack(144); + TEST(ret == 0, + "test_with_small_stack failed, returned {} with 144-byte stack", + ret); + ret = test_with_small_stack(112); + TEST(ret == -ENOTENOUGHSTACK, + "test_with_small_stack failed, returned {} with 112-byte stack", + ret); __cheri_callback void (*callback)() = cross_compartment_call; crossCompartmentCall = test_trusted_stack_exhaustion; @@ -127,5 +176,6 @@ debug_log("invalid stack on cross compartment call"); expect_handler(false); + test_stack_invalid_on_call(callback); }
diff --git a/tests/stack_integrity_thread.cc b/tests/stack_integrity_thread.cc index dad3f42..7a629c5 100644 --- a/tests/stack_integrity_thread.cc +++ b/tests/stack_integrity_thread.cc
@@ -139,3 +139,8 @@ { self_recursion(fn); } + +int test_stack_requirement() +{ + return 0; +}
diff --git a/tests/stack_tests.h b/tests/stack_tests.h index bfbe74b..fbc757b 100644 --- a/tests/stack_tests.h +++ b/tests/stack_tests.h
@@ -52,3 +52,6 @@ return false; } + +__cheri_compartment("stack_integrity_thread") + __cheriot_minimum_stack(128) int test_stack_requirement();
diff --git a/tests/test-runner.cc b/tests/test-runner.cc index 9d1452d..d96a8e0 100644 --- a/tests/test-runner.cc +++ b/tests/test-runner.cc
@@ -52,6 +52,9 @@ #endif return ErrorRecoveryBehaviour::ForceUnwind; } + debug_log("mcause: {}, pcc: {}", mcause, frame->pcc); + auto [reg, cause] = CHERI::extract_cheri_mtval(mtval); + debug_log("Error {} in register {}", reg, cause); debug_log("Current test crashed"); crashDetected = true; return ErrorRecoveryBehaviour::InstallContext;