Don't invoke error handlers when the callee errors.

I originally made a forced unwind out of a compartment invoke the
caller's error handler (if it existed) with a special error code.  The
idea was that you could use error handlers to jump to recovery paths,
rather than checking return values.

This seemed like a good idea at the time, but turned out to be a
mistake.  The only code that has ever used this behaviour was the test
suite.  Other error handlers have special-case logic to check if this is
the reason and ignore it if so.  It also added a lot of confusing
control flow in the switcher, which is bad because the switcher is the
core of the TCB.

Fixes #302

sdk/core/switcher/entry.S

@@ -485,82 +485,44 @@ exception_entry_asm:
 	csrw               CSR_MSHWMB, x1
 #endif
 	cspecialw          mepcc, ct2
-	csb                zero, TrustedStack_offset_inForcedUnwind(csp)
 	// c2 is csp, which will be loaded last and will overwrite the trusted
 	// stack pointer with the thread's stack pointer.
 	reloadRegisters c1, cgp, c4, c5, c6, c7, c8, c9, c10, c11, c12, c13, c14, c15, csp
 	mret
 
-// If we detect an invalid entry and there is no error handler installed, we want
-// to resume rather than unwind.
-.Linvalid_entry:
-// Mark this threads as in the middle of a forced unwind.
-	li                 a0, 1
-	csb                a0, TrustedStack_offset_inForcedUnwind(ctp)
-// Make sure we don't leak anything to the compartment.
-// Registers might been used by the call and therefore need zeroing.
-	zeroAllRegistersExcept a0, s0, s1, sp, a2, gp
-// Store an error value in return registers, which will be passed to the
-// caller on unwind. a1 is zeroed by zeroAllRegistersExcept.
-	li                 a0, -1
 // We are starting a forced unwind.  This is reached either when we are unable
 // to run an error handler, or when we do run an error handler and it instructs
 // us to return.  This treats all register values as undefined on entry.
 .Lforce_unwind:
 	// Pop the trusted stack frame.
 	cjal               .Lpop_trusted_stack_frame
 	cmove              cra, ca2
+	// Zero all registers apart from RA, GP, SP and return args.
+	// cra, cs0, cs1, and cgp were restored from the compartment's stack
+	// csp restored from the trusted stack.
+	// ca0, used for first return value
+	// ca1, used for second return value
+	zeroAllRegistersExcept ra, sp, gp, s0, s1, a0, a1
+	li                 a0, -ECOMPARTMENTFAIL
+	li                 a1, 0
+	cret
+
+
+// If we have run out of trusted stack, then just restore the caller's state
+// and return an error value.
 .Lout_of_trusted_stack:
-	cmove              ct0, csp
-	// Fetch the trusted stack pointer.
-	cspecialr          csp, mtdc
-	// csp now points to the save reg frame that we can use.
-	// Spill all of the registers that we want to propagate to the caller:
-	// c1(cra), c2(csp), c3(cgp), c8(cs0), c9(cs1), c10(ca0), c11(ca1)
-	csc                ct0, TrustedStack_offset_csp(csp)
-	spillRegisters c1, cgp, c8, c9, c10, c11
-	// Store an unsealed version of cra in the mepcc slot, where it will be
-	// used for mret later.  mret requires an unsealed capability in mepcc, so
-	// we have to unseal it if it is sealed.
-	LoadCapPCC         cs0, compartment_switcher_sealing_key
-	// ca2 at this point was loaded by .Lpop_trusted_stack_frame from the pcc
-	// in the trusted stack and so should always be sealed as a sentry type.
-	cgettype           gp, cra
-	csetaddr           cs0, cs0, gp
-	cunseal            cra, cra, cs0
-	csc                cra, TrustedStack_offset_mepcc(csp)
-	clw                t0, TrustedStack_offset_mstatus(csp)
-	// If gp==2 then the we need to disable interrupts on return, otherwise we
-	// need to enable them.  The interrupt enable bit is bit 7.  We want to set
-	// bit 7 if interrupts are enabled, clear it if they are disabled, but not
-	// toggle any other bits.
-	// Clear the interrupt enable bit unconditionally
-	andi               t0, t0, ~0x80
-	// Set it again if we should have interrupts enabled
-	li                 a3, 2
-	beq                gp, a3, .Ldo_not_enable
-	ori                t0, t0, 0x80
-.Ldo_not_enable:
-	csw                t0, TrustedStack_offset_mstatus(csp)
-
-	// Zero all registers that we aren't explicitly restoring to avoid leaks
-	// from the faulting callee to the caller.
-	csc                cnull, TrustedStack_offset_c4(csp)
-	csc                cnull, TrustedStack_offset_c5(csp)
-	csc                cnull, TrustedStack_offset_c6(csp)
-	csc                cnull, TrustedStack_offset_c7(csp)
-	csc                cnull, TrustedStack_offset_c12(csp)
-	csc                cnull, TrustedStack_offset_c13(csp)
-	csc                cnull, TrustedStack_offset_c14(csp)
-	csc                cnull, TrustedStack_offset_c15(csp)
-	// Mark this threads as in the middle of a forced unwind.
-	li                 a0, 1
-	csb                a0, TrustedStack_offset_inForcedUnwind(csp)
-	// Spill a fake status and cap cause (CHERI fault, no cause)
-	li                 a0, 0x1c
-	csw                a0, TrustedStack_offset_mcause(csp)
-	csrw               mtval, zero
-	// Fall through to handle error
+	// Restore the spilled values
+	clc                cs0, SPILL_SLOT_cs0(csp)
+	clc                cs1, SPILL_SLOT_cs1(csp)
+	clc                cra, SPILL_SLOT_pcc(csp)
+	clc                cgp, SPILL_SLOT_cgp(csp)
+	cincoffset         csp, csp, SPILL_SLOT_SIZE
+	// Set the return registers
+	li                 a0, -ENOTENOUGHTRUSTEDSTACK
+	li                 a1, 0
+	// Zero everything else
+	zeroAllRegistersExcept ra, sp, gp, s0, s1, a0, a1
+	cret
 
 // If we have a possibly recoverable error, see if we have a useful error
 // handler.  At this point, the register state will have been saved in the
@@ -600,7 +562,7 @@ exception_entry_asm:
 	// See if we can find a handler:
 	clhu               tp, TrustedStack_offset_frameoffset(csp)
 	li                 t1, TrustedStack_offset_frames
-	beq                tp, t1, .Lend_of_stack
+	beq                tp, t1, .Lreset_mepcc_and_install_context
 	addi               tp, tp, -TrustedStackFrame_size
 
 	// ctp points to the current available trusted stack frame.
@@ -649,7 +611,7 @@ exception_entry_asm:
 	// A value of 0xffff indicates no error handler
 	// Give up if there is no error handler for this compartment.
 	li                 s1, 0xffff
-	beq                s0, s1, .Lno_handler_found
+	beq                s0, s1, .Lforce_unwind
 
 	// The stack may have had its tag cleared at this point, so for stackless
 	// handlers we need to restore the on-entry stack.
@@ -677,10 +639,6 @@ exception_entry_asm:
 
 .Lhandler_found:
 
-	// If we have found a handler, mark this threads as no longer on the
-	// force-unwind path.  Any future fault will trigger a forced unwind.
-	csb                zero, TrustedStack_offset_inForcedUnwind(csp)
-
 	// Increment the handler invocation count.
 	clhu               s1, TrustedStackFrame_offset_errorHandlerCount(ctp)
 	addi               s1, s1, 1
@@ -816,23 +774,11 @@ exception_entry_asm:
 	j                  .Lhandle_error
 
 
-// We have reached the end of the stack.  If we are in a forced unwind then we
-// just install the context, if we've gone off the top of the stack then we
-// should report this gracefully.
-.Lend_of_stack:
-	clb                a2, TrustedStack_offset_inForcedUnwind(csp)
-	bnez               a2, .Lreset_mepcc_and_install_context
 	// Value 24 is reserved for custom use.
 .Lset_mcause_and_exit_thread:
 	csrw               mcause, 24
 	j                  .Lthread_exit
 
-// No handler was found.  If we are in the middle of unwinding, then we want to
-// just install the context but if this is a fault then we keep going up the
-// stack.
-.Lno_handler_found:
-	clb                a2, TrustedStack_offset_inForcedUnwind(csp)
-	beqz               a2, .Lforce_unwind
 	// The continue-resume path expects the location that we will mret to to be
 	// in ct2.  If we're just resuming, then resume from the stashed link
 	// register value.

sdk/core/switcher/trusted-stack-assembly.h

@@ -28,12 +28,12 @@ EXPORT_ASSEMBLY_OFFSET(TrustedStack, mshwmb, (18 * 8) + 4)
 
 // Size of everything up to this point
 #	define TSTACK_REGFRAME_SZ (19 * 8)
-// frameoffset, inForcedUnwind and padding
+// frameoffset and padding
 #	define TSTACK_HEADER_SZ 16
 #else
 // Size of everything up to this point
 #	define TSTACK_REGFRAME_SZ ((17 * 8) + (2 * 4))
-// frameoffset, inForcedUnwind and padding
+// frameoffset and padding
 #	define TSTACK_HEADER_SZ 8
 #endif
 // The basic trusted stack is the size of the save area, 8 bytes of state for
@@ -47,7 +47,6 @@ EXPORT_ASSEMBLY_OFFSET(TrustedStack,
                        TSTACK_REGFRAME_SZ + TSTACK_HEADER_SZ)
 EXPORT_ASSEMBLY_OFFSET(TrustedStack, frameoffset, TSTACK_REGFRAME_SZ)
 EXPORT_ASSEMBLY_OFFSET(TrustedStack, threadID, TSTACK_REGFRAME_SZ + 2)
-EXPORT_ASSEMBLY_OFFSET(TrustedStack, inForcedUnwind, TSTACK_REGFRAME_SZ + 4)
 
 EXPORT_ASSEMBLY_OFFSET(TrustedStackFrame, csp, 0)
 EXPORT_ASSEMBLY_OFFSET(TrustedStackFrame, calleeExportTable, 8)

sdk/core/switcher/tstack.h

@@ -59,17 +59,12 @@ struct TrustedStackGeneric
 	 * The ID of the current thread.  Never modified during execution.
 	 */
 	uint16_t threadID;
-	/**
-	 * Flag indicating whether this thread is in the process of a forced
-	 * unwind.  If so, this is one, otherwise it is zero.
-	 */
-	uint8_t inForcedUnwind;
 	// Padding up to multiple of 16-bytes.
 	uint8_t padding[
 #ifdef CONFIG_MSHWM
-	  11
+	  12
 #else
-	  3
+	  4
 #endif
 	];
 	/**

sdk/include/errno.h

@@ -89,6 +89,7 @@
 #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 ENOTENOUGHTRUSTEDSTACK 141 // 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.

sdk/include/fail-simulator-on-error.h

@@ -67,11 +67,6 @@ compartment_error_handler(ErrorState *frame, size_t mcause, size_t mtval)
 			DebugErrorHandler::log(
 			  "Thread exit CSP={}, PCC={}", stackCapability, frame->pcc);
 		}
-		else if (exceptionCode == CHERI::CauseCode::None)
-		{
-			// An unwind occurred from a called compartment, just resume.
-			return ErrorRecoveryBehaviour::InstallContext;
-		}
 		else
 		{
 			// An unexpected error -- log it and end the simulation

tests/allocator-test.cc

@@ -615,7 +615,7 @@ namespace
 /**
  * Allocator test entry point.
  */
-void test_allocator()
+int test_allocator()
 {
 	GlobalConstructors::run();
 
@@ -682,4 +682,5 @@ void test_allocator()
 	TEST(quotaLeft == MALLOC_QUOTA,
 	     "After alloc and free from 0x100000-byte quota, {} bytes left",
 	     quotaLeft);
+	return 0;
 }

tests/check_pointer-test.cc

@@ -128,7 +128,8 @@ void check_pointer_strict_mode(int *ptr)
 	     "with a raw capability.");
 }
 
-void test_check_pointer()
+int test_check_pointer()
 {
 	check_pointer_strict_mode(&object);
+	return 0;
 }

tests/compartment_calls-test.cc

@@ -44,7 +44,7 @@ void test_number_of_arguments()
 	TEST(ret == 0, "compartment_call_inner returend {}", ret);
 }
 
-void test_compartment_call()
+int test_compartment_call()
 {
 	bool outTestFailed = false;
 	int  ret           = 0;
@@ -67,10 +67,9 @@ void test_compartment_call()
 	     csp);
 
 	test_number_of_arguments();
-	ret = test_incorrect_export_table_with_handler(nullptr);
-	TEST(ret == -1, "Test incorrect entry point with error handler failed");
 
 	test_incorrect_export_table(nullptr, &outTestFailed);
 	TEST(outTestFailed == false,
 	     "Test incorrect entry point without error handler failed");
+	return 0;
 }

tests/compartment_calls.h

@@ -43,6 +43,6 @@ __cheri_compartment("compartment_calls_inner") void test_incorrect_export_table(
   bool *outTestFailed);
 __cheri_compartment(
   "compartment_calls_inner_with_"
-  "handler") int test_incorrect_export_table_with_handler(__cheri_callback void (*fn)());
+  "handler") int test_incorrect_export_table_with_handler(__cheri_callback int (*fn)());
 __cheri_compartment("compartment_calls_outer") void compartment_call_outer();
-constexpr int ConstantValue = 0x41414141;
+constexpr int ConstantValue = 0x41414141;

tests/crash_recovery-test.cc

@@ -36,7 +36,7 @@ compartment_error_handler(struct ErrorState *frame, size_t mcause, size_t mtval)
 	return ErrorRecoveryBehaviour::InstallContext;
 }
 
-void test_crash_recovery()
+int test_crash_recovery()
 {
 	debug_log("Calling crashy compartment indirectly");
 	test_crash_recovery_outer(0);
@@ -46,30 +46,34 @@ void test_crash_recovery()
 	          "nested call crashed");
 
 	debug_log("Calling crashy compartment to fault and unwind");
-	test_crash_recovery_inner(0);
+	void *ret = test_crash_recovery_inner(0);
 	check_stack();
-	debug_log("Calling crashy compartment returned (crashes: {})", crashes);
-	TEST(crashes == 1, "Failed to notice crash");
+	debug_log("Calling crashy compartment returned ({})", ret);
+	TEST(crashes == 0, "Should not have crashed");
+	TEST(ret != nullptr, "Failed to notice crash");
 
 	debug_log("Calling crashy compartment to return normally");
-	test_crash_recovery_inner(1);
+	ret = test_crash_recovery_inner(1);
 	check_stack();
 	debug_log("Calling crashy compartment returned (crashes: {})", crashes);
-	TEST(crashes == 1, "Should not have crashed");
+	TEST(crashes == 0, "Should not have crashed");
+	TEST(ret == nullptr, "Failed to notice crash");
 	debug_log("Returning normally from crash test");
 
 	debug_log("Calling crashy compartment to double fault and unwind");
-	test_crash_recovery_inner(2);
+	ret = test_crash_recovery_inner(2);
 	check_stack();
 	debug_log("Calling crashy compartment returned (crashes: {})", crashes);
-	TEST(crashes == 2, "Failed to notice crash");
+	TEST(crashes == 0, "Should not have crashed");
+	TEST(ret != nullptr, "Failed to notice crash");
 
 	debug_log(
 	  "Calling crashy compartment to corrupt CSP in stack pointer and unwind");
-	test_crash_recovery_inner(3);
+	ret = test_crash_recovery_inner(3);
 	check_stack();
 	debug_log("Calling crashy compartment returned (crashes: {})", crashes);
-	TEST(crashes == 3, "Failed to notice crash");
+	TEST(crashes == 0, "Should not have crashed");
+	TEST(ret != nullptr, "Failed to notice crash");
 
 	ptraddr_t handlerCount = switcher_handler_invocation_count_reset();
 	TEST(handlerCount == crashes * 2,
@@ -90,5 +94,6 @@ void test_crash_recovery()
 		// does not return and so will not generate code following it.
 		asm volatile("c.unimp");
 	}
-	TEST(crashes == 3 + MaxCrashes, "Failed to notice crash");
+	TEST(crashes == MaxCrashes, "Failed to notice crash");
+	return 0;
 }

tests/debug-test.c

@@ -1,7 +1,7 @@
 #include <compartment.h>
 #include <debug.h>
 
-__cheri_compartment("debug_test") void test_debug_c()
+__cheri_compartment("debug_test") int test_debug_c()
 {
 	unsigned char x = 'c';
 	_Bool         t = true;
@@ -22,4 +22,5 @@ __cheri_compartment("debug_test") void test_debug_c()
 	CHERIOT_INVARIANT(true, "Testing C++ invariant failure");
 	CHERIOT_INVARIANT(
 	  true, "Testing C++ invariant failure: 42:{}", 42, 1, 3, 4, "oops");
+	return 0;
 }