Add APIs for checking stack usage. This makes it possible to dynamically check the maximum stack usage for a compartment entry point in testing. This is intended to be used in conjunction with CHERIoT-Platform/llvm-project#22, which makes it possible to specify the minimum stack size (enforced by the switcher). Also expose the attribute via a macro and use it in the allocator. The allocator parts demonstrate a recommended way of using the dynamic checks: each allocator function has its stack usage recorded and, in release builds for the test suite, we make the compartment crash if it used more stack than it expected. Adding more tests will make this more robust.
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 660447c..736ca36 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml
@@ -17,7 +17,7 @@ - build-type: debug build-flags: --debug-loader=y --debug-scheduler=y --debug-allocator=y -m debug - build-type: release - build-flags: --debug-loader=n --debug-scheduler=n --debug-allocator=n -m release + build-flags: --debug-loader=n --debug-scheduler=n --debug-allocator=n -m release --stack-usage-check-allocator=y fail-fast: false runs-on: ubuntu-latest container:
diff --git a/sdk/core/allocator/alloc_config.h b/sdk/core/allocator/alloc_config.h index a032fd2..522ed8a 100644 --- a/sdk/core/allocator/alloc_config.h +++ b/sdk/core/allocator/alloc_config.h
@@ -23,3 +23,16 @@ constexpr size_t MallocAlignment = 1U << MallocAlignShift; constexpr size_t MallocAlignMask = MallocAlignment - 1; + +constexpr StackCheckMode StackMode = +#if CHERIOT_STACK_CHECKS_ALLOCATOR + StackCheckMode::Asserting +#else + StackCheckMode::Disabled +// Uncomment if checks failed to find the correct values +// StackCheckMode::Logging +#endif + ; + +#define STACK_CHECK(expected) \ + StackUsageCheck<StackMode, expected, __PRETTY_FUNCTION__> stackCheck
diff --git a/sdk/core/allocator/main.cc b/sdk/core/allocator/main.cc index a7364b6..58b6ea1 100644 --- a/sdk/core/allocator/main.cc +++ b/sdk/core/allocator/main.cc
@@ -803,8 +803,10 @@ } // namespace -size_t heap_quota_remaining(struct SObjStruct *heapCapability) +__cheriot_minimum_stack(0x80) size_t + heap_quota_remaining(struct SObjStruct *heapCapability) { + STACK_CHECK(0x80); LockGuard g{lock}; auto *cap = malloc_capability_unseal(heapCapability); if (cap == nullptr) @@ -814,8 +816,9 @@ return cap->quota; } -void heap_quarantine_empty() +__cheriot_minimum_stack(0xb0) void heap_quarantine_empty() { + STACK_CHECK(0xb0); LockGuard g{lock}; while (gm->heapQuarantineSize > 0) { @@ -829,8 +832,11 @@ } } -void *heap_allocate(Timeout *timeout, SObj heapCapability, size_t bytes) +__cheriot_minimum_stack(0x1f0) void *heap_allocate(Timeout *timeout, + SObj heapCapability, + size_t bytes) { + STACK_CHECK(0x1f0); if (!check_timeout_pointer(timeout)) { return nullptr; @@ -850,8 +856,10 @@ return malloc_internal(bytes, std::move(g), cap, timeout); } -size_t heap_claim(SObj heapCapability, void *pointer) +__cheriot_minimum_stack(0x1b0) size_t + heap_claim(SObj heapCapability, void *pointer) { + STACK_CHECK(0x1b0); LockGuard g{lock}; auto *cap = malloc_capability_unseal(heapCapability); if (cap == nullptr) @@ -878,14 +886,18 @@ return 0; } -int heap_can_free(SObj heapCapability, void *rawPointer) +__cheriot_minimum_stack(0xe0) int heap_can_free(SObj heapCapability, + void *rawPointer) { + STACK_CHECK(0xe0); LockGuard g{lock}; return heap_free_internal(heapCapability, rawPointer, false); } -int heap_free(SObj heapCapability, void *rawPointer) +__cheriot_minimum_stack(0x250) int heap_free(SObj heapCapability, + void *rawPointer) { + STACK_CHECK(0x250); LockGuard g{lock}; int ret = heap_free_internal(heapCapability, rawPointer, true); if (ret != 0) @@ -904,8 +916,9 @@ return 0; } -ssize_t heap_free_all(SObj heapCapability) +__cheriot_minimum_stack(0x180) ssize_t heap_free_all(SObj heapCapability) { + STACK_CHECK(0x180); LockGuard g{lock}; auto *capability = malloc_capability_unseal(heapCapability); if (capability == nullptr) @@ -942,11 +955,12 @@ return freed; } -void *heap_allocate_array(Timeout *timeout, - SObj heapCapability, - size_t nElements, - size_t elemSize) +__cheriot_minimum_stack(0x1f0) void *heap_allocate_array(Timeout *timeout, + SObj heapCapability, + size_t nElements, + size_t elemSize) { + STACK_CHECK(0x1f0); if (!check_timeout_pointer(timeout)) { return nullptr; @@ -1087,12 +1101,14 @@ return nullptr; } -SObj token_sealed_unsealed_alloc(Timeout *timeout, - SObj heapCapability, - SKey key, - size_t sz, - void **unsealed) +__cheriot_minimum_stack(0x250) SObj + token_sealed_unsealed_alloc(Timeout *timeout, + SObj heapCapability, + SKey key, + size_t sz, + void **unsealed) { + STACK_CHECK(0x250); if (!check_timeout_pointer(timeout)) { return INVALID_SOBJ; @@ -1112,11 +1128,12 @@ return INVALID_SOBJ; } -SObj token_sealed_alloc(Timeout *timeout, - SObj heapCapability, - SKey rawKey, - size_t sz) +__cheriot_minimum_stack(0x250) SObj token_sealed_alloc(Timeout *timeout, + SObj heapCapability, + SKey rawKey, + size_t sz) { + STACK_CHECK(0x250); return allocate_sealed_unsealed( timeout, heapCapability, rawKey, sz, {Permission::Seal}) .first; @@ -1149,8 +1166,11 @@ return unsealed; } -int token_obj_destroy(SObj heapCapability, SKey key, SObj object) +__cheriot_minimum_stack(0x250) int token_obj_destroy(SObj heapCapability, + SKey key, + SObj object) { + STACK_CHECK(0x250); void *unsealed; { LockGuard g{lock}; @@ -1168,8 +1188,11 @@ return heap_free(heapCapability, unsealed); } -int token_obj_can_destroy(SObj heapCapability, SKey key, SObj object) +__cheriot_minimum_stack(0xe0) int token_obj_can_destroy(SObj heapCapability, + SKey key, + SObj object) { + STACK_CHECK(0xe0); void *unsealed; { LockGuard g{lock};
diff --git a/sdk/core/switcher/entry.S b/sdk/core/switcher/entry.S index 93baf4a..13b6523 100644 --- a/sdk/core/switcher/entry.S +++ b/sdk/core/switcher/entry.S
@@ -952,6 +952,13 @@ cret + .section .text, "ax", @progbits + .p2align 2 + .type __Z25stack_lowest_used_addressv,@function +__Z25stack_lowest_used_addressv: + // Read the stack high-water mark into the return register. + csrr a0, CSR_MSHWM + cret // The linker expects export tables to start with space for cgp and pcc, then // the compartment error handler. We should eventually remove that requirement @@ -986,3 +993,4 @@ export __Z23switcher_current_threadv export __Z28switcher_thread_hazard_slotsv export __Z13thread_id_getv +export __Z25stack_lowest_used_addressv
diff --git a/sdk/include/cdefs.h b/sdk/include/cdefs.h index b1f30dc..d7e87e6 100644 --- a/sdk/include/cdefs.h +++ b/sdk/include/cdefs.h
@@ -62,6 +62,7 @@ #define __alloc_size(x) __attribute__((alloc_size(x))) #define __alloc_align(x) __attribute__((alloc_align(x))) #define __cheri_callback __attribute__((cheri_ccallback)) +#define __cheriot_minimum_stack(x) __attribute__((cheriot_minimum_stack(x))) // When running clang-tidy, we use the same compile flags for everything and so // will get errors about things being defined in the wrong compartment, so // define away the compartment name and pretend everything is local for now.
diff --git a/sdk/include/debug.hh b/sdk/include/debug.hh index d107a78..a95470b 100644 --- a/sdk/include/debug.hh +++ b/sdk/include/debug.hh
@@ -2,13 +2,13 @@ // SPDX-License-Identifier: MIT #pragma once -#include "cdefs.h" #include <cheri.hh> #include <compartment.h> #include <concepts> #include <cstddef> #include <platform-uart.hh> #include <string.h> +#include <switcher.h> #include <array> #include <string_view> @@ -741,4 +741,92 @@ Assert(auto, const char *, Ts &&...) -> Assert<Ts...>; }; + enum class StackCheckMode + { + Disabled, + Logging, + Asserting, + }; + + /** + * Check the (dynamic) stack usage of a function, including all of its + * callees. This is intended to be used in compartment entry points to + * check the stack size that is required for that entry point. Use this + * with some test vectors that explore different code paths to identify the + * maximum stack usage. This can then be used with the + * [[cheriot::minimum_stack]] attribute to ensure that the switcher will + * never invoke the entry point with insufficient stack. + * + * Note that this records only the stack usage for the current compartment + * invocation (including any invoked shared libraries). If this + * compartment calls others, then a larger stack may be required. This + * failure is recoverable: cross-compartment calls should always be assumed + * to potentially fail. This check is to ensure that an attacker cannot + * cause stack overflow to crash the compartment, not to ensure that an + * attacker cannot cause stack overflow to make the operation that they + * requested fail. + * + * Stack checks run in one of three modes: + * + * - Disabled: The checks do not run. + * - Logging: The checks run and print a message if the stack usage + * exceeds expectations. + * - Asserting: After printing the message, the function will trap. + * + * In logging mode, one message will be printed for each invocation of the + * function that exceeds the previous maximum stack usage. + * + * An instance of this should be created as a local variable on entry to a + * function. The destructor (which prints the message) will then run after + * any other destructors, ensuring the most accurate stack usage + * measurement. + * + * Unfortunately, default arguments for templates are not evaluated in the + * enclosing scope and so `__builtin_FUNCTION()` cannot be used here. + * Instead, we must pass this explicitly, typically as something like: + * + * ```c++ + * StackUsageCheck<DebugFlag, 128, __PRETTY_FUNCTION__> stackCheck; + * ``` + */ + template<StackCheckMode Mode, size_t Expected, DebugContext Fn> + class StackUsageCheck + { + /** + * The expected maximum. This class is templated on the function name + * and so there is one copy of this per function. + */ + static inline size_t stackUsage = Expected; + + public: + /** + * Default constructor, does nothing. + */ + StackUsageCheck() = default; + + /** + * Destructor, runs at the end of the function to print the message. + */ + __always_inline ~StackUsageCheck() + { + if constexpr (Mode != StackCheckMode::Disabled) + { + ptraddr_t lowest = stack_lowest_used_address(); + ptraddr_t highest = + CHERI::Capability{__builtin_cheri_stack_get()}.top(); + size_t used = highest - lowest; + if (used > stackUsage) + { + stackUsage = used; + ConditionalDebug<true, Fn>::log("Stack used: {} bytes", + stackUsage); + if constexpr (Mode == StackCheckMode::Asserting) + { + __builtin_trap(); + } + } + } + } + }; + } // namespace
diff --git a/sdk/include/switcher.h b/sdk/include/switcher.h index ad65a42..a1088b6 100644 --- a/sdk/include/switcher.h +++ b/sdk/include/switcher.h
@@ -1,5 +1,6 @@ #pragma once #include <cdefs.h> +#include <stddef.h> /** * Returns true if the trusted stack contains at least `requiredFrames` frames @@ -49,3 +50,9 @@ * next cross-compartment call or until they are explicitly overwritten. */ __cheri_libcall void **switcher_thread_hazard_slots(void); + +/** + * Returns the lowest address that has been stored to on the stack in this + * compartment invocation. + */ +__cheri_libcall ptraddr_t stack_lowest_used_address(void);
diff --git a/sdk/xmake.lua b/sdk/xmake.lua index fb74db4..1d763bc 100644 --- a/sdk/xmake.lua +++ b/sdk/xmake.lua
@@ -39,6 +39,17 @@ debugOption("allocator") debugOption("token_library") +function stackCheckOption(name) + option("stack-usage-check-" .. name) + set_default(false) + set_description("Enable dynamic stack usage checks in " .. name .. ". Do not enable this in debug builds!") + set_showmenu(true) + set_category("Debugging") + option_end() +end + +stackCheckOption("allocator") + -- Force -Oz irrespective of build config. At -O0, we blow out our stack and -- require much stronger alignment. set_optimize("Oz") @@ -199,7 +210,7 @@ -- having an allocator (or into providing a different allocator for a -- particular application) target("cheriot.allocator") - add_rules("cheriot.privileged-compartment", "cheriot.component-debug") + add_rules("cheriot.privileged-compartment", "cheriot.component-debug", "cheriot.component-stack-checks") add_files(path.join(coredir, "allocator/main.cc")) add_deps("locks") add_deps("compartment_helpers") @@ -715,6 +726,13 @@ target:add('defines', "DEBUG_" .. name:upper() .. "=" .. tostring(get_config("debug-"..name))) end) +-- Rule for conditionally enabling stack checks for a component. +rule("cheriot.component-stack-checks") + after_load(function (target) + local name = target:get("cheriot.debug-name") or target:name() + target:add('options', "stack-usage-check-" .. name) + target:add('defines', "CHERIOT_STACK_CHECKS_" .. name:upper() .. "=" .. tostring(get_config("stack-usage-check-"..name))) + end) -- Build the loader. The firmware rule will set the flags required for -- this to create threads.
diff --git a/tests/allocator-test.cc b/tests/allocator-test.cc index dcc7191..2bf3926 100644 --- a/tests/allocator-test.cc +++ b/tests/allocator-test.cc
@@ -542,7 +542,20 @@ TEST(heap_address_is_valid(&noWait) == false, "Global object incorrectly reported as heap address"); + t = 5; + Capability array{heap_allocate_array(&t, MALLOC_CAPABILITY, 0x80000004, 2)}; + TEST( + !array.is_valid(), "Allocating too large an array succeeded: {}", array); + array = heap_allocate_array(&t, MALLOC_CAPABILITY, 16, 2); + TEST(array.is_valid(), "Allocating array failed: {}", array); + TEST(array.length() == 32, + "Allocating array returned incorrect length: {}", + array); + ret = heap_free(MALLOC_CAPABILITY, array); + TEST(ret == 0, "Freeing array failed: {}", ret); + test_blocking_allocator(); + heap_quarantine_empty(); test_revoke(); test_fuzz(); allocations.clear();