WIP: various tweaks to, and a pile of documentation for, the switcher and exception handler #320

nwf · 2024-10-18T23:12:48Z

The switcher is a very small, and yet also big, chunk of carefully hand-written, security-critical assembler. In absence of formal verification of correctness, add more commentary about what's going on.

At the time of writing, this just adds "Register Atlas"es to the __Z26compartment_switcher_entryz entry point (and very slightly moves some code around).

I would also like to have "Control Flow Atlas"es with backwards arcs from labels with interesting control flow, but I haven't gotten there yet.

Mostly posting for bikeshedding and commentary; commit early, commit often.

sdk/core/switcher/entry.S

nwf · 2024-10-25T03:50:08Z

OK, I believe this is ready for review. As so often happens with my PRs, this is somewhat separable, with many of the "bottom" commits suitable for landing even if the whole thing is not, if that'd be desirable. In the same spirit, this is almost surely best reviewed commit by commit, rather than all at once.

davidchisnall

Can you comment out the labels that are not branch targets? I found that they made the code harder to read, whereas everything else was a big improvement.

sdk/core/switcher/entry.S

davidchisnall · 2024-10-25T09:35:28Z

sdk/core/switcher/entry.S

+	 *  tp, t2: scratch
+	 */
+	/*
+	 * The caller should back up all caller saved registers.  Spill


This took a couple of attempts to parse. The caller is asserting that they don’t care about the values in caller-save registers. There is no obligation to spill them, they may simply be unused.

Please check that you're happy with the new comment here.

davidchisnall · 2024-10-25T09:37:25Z

sdk/core/switcher/entry.S

+	 * callee-save registers carefully.  If we find ourselves unable to do
+	 * so, we'll return an error to the caller (via the exception path; see
+	 * .Lhandle_error_in_switcher).  The error handling path assumes that
+	 * the first spill is to the lowest address and guaranteed to trap if


I don’t think that’s a safe assumption. What happens if:

I clear store global from CSP and don’t have local things in cs0?

I put a heap cap in CSP and free it in another thread? I think this can’t happen yet but will be possible once the switcher runs with interrupts enabled (or we do multicore).

After more thinking, I think the code is right but the comment is misleading. If we've accidentally run out of stack space, we'll trap on the first instruction and so should gracefully return. If we've maliciously done bad things to csp before invoking the switcher, we should be force unwound.

davidchisnall · 2024-10-25T09:38:30Z

sdk/core/switcher/entry.S

+	 */
+	cgetperm           t2, csp
+	li                 tp, COMPARTMENT_STACK_PERMISSIONS
+	bne                tp, t2, .Lforce_unwind


Not that this check checks for store local which ensures that this is a stack and so later accesses cannot fault.

Please check that you are happy with the expanded comment.

sdk/core/switcher/entry.S

davidchisnall · 2024-10-25T13:21:37Z

sdk/core/switcher/entry.S

+	 * If we have already unwound so far that the TrustedStack::frameoffset is
+	 * pointing at TrustedStack::frames[0] -- that is, if the stack has no
+	 * active frames on it -- then just go back to the context we came from,
+	 * effectively parking this thread in a (slow) infinite loop.


Don't we want to hit the thread exit path in that case?

I think we already have hit the thread exit path in this case, and the scheduler is continuing to try to run us. But all the same, see #328 (not presently part of this PR stack, but if it lands, I'll rebase).

sdk/core/switcher/entry.S

davidchisnall · 2024-10-25T13:36:13Z

sdk/core/switcher/entry.S

 	cspecialr          ct1, mtdc
+	// Atlas: t1: pointer to TrustedStack
 #ifdef CONFIG_MSHWM


Maybe not this PR, but I think we can assume MSHWM now. We haven't tested without it for ages and I expect everyone to use it.

The ifdef was useful for benchmarking, not sure if you still want that?

hlef · 2024-10-29T16:39:06Z

Just mentioning here: I encountered the bug where a compartment's error handler isn't called if the thread is at the root of the trusted stack while testing my future PR that uses the new error handlers in the NW stack (https://github.com/CHERIoT-Platform/network-stack/tree/hlefeuvre/stack-overflow-resilient-error-handler). I applied this patch to my local tree and it fixed it.

#320 is taking a while since it's a sizable amount of prose. Pull the end-user visible changes off the bottom and make them their own PR. --------- Co-authored-by: Robert Norton <robert.norton@microsoft.com> Co-authored-by: Murali Vijayaraghavan <vmurali@csail.mit.edu>

nwf · 2024-11-13T05:21:28Z

This is sitting atop #346 at the moment (which hopefully lands quickly). The dot_from_switcher.lua script has grown the ability to cross-check not just FROM/IFROM/ITO annotations but also IRQ ASSUME and IRQ REQUIRE. Assuming I have gotten the script and the latter annotations right, there are no more IRQ-related surprises lurking in the switcher.

I would very much appreciate more eyes / the same eyes again on this so that it can hopefully land soon.

Co-authored-by: David Chisnall <github@theravensnest.org> Co-authored-by: Robert Norton <robert.norton@microsoft.com>

Co-authored-by: Robert Norton <robert.norton@microsoft.com>

Inline check_compartment_stack_integrity since it's used exactly once and makes some assumptions (and had a bug, hard-coding `csp` when it meant `\reg`). Remove the `csp`-relative `lb`, as we have already checked the things that this would that the surviving subsequent permissions check would not. Leave the permission and alignedness checks.

Force unwind clobbers a0 and a1; there's no need to be paranoid about their values before possibly trapping or manually invoking force unwind behavior.

.Lout_of_trusted_stack is used only by __Z26compartment_switcher_entryz, so move it up closer to that, next to the other local error paths.

When loading the target thread's trusted stack, do more work in csp, just so we don't have to explain that ct0 gets clobbered by something else in the next instruction. This may be excessive.

When in .Lskip_compartment_call, t0 is spare, in that it will be overwritten by one of... - us, in a few instructions (the zeroAllRegistersExcept), or - zeroAllRegistersExcept in .Ltherad_exit, via .Lset_mcause_and_exit_thread, or - the exception path (.Lforce_unwind via .Lhandle_error_in_switcher) bringing us back here, one TrustedFrame further up. We can use it instead of tp for a temporary value, avoiding the need to reload mtdc.

Having spilled s0 already, we're free to use it rather than clobber t2. We'll zero t2 on the error path here, so there is no concern of it leaking.

These were only useful on simulators that did not report all effects of instructions, and we no longer use any such.

@davidchisnall

As @davidchisnall points out, just because sp's address is zero doesn't mean that the rest of the cap is too. So, if it looks like we're dead in the water, make sure we're actually dead by zeroing mtcc completely. While here, slightly tidy the code by flipping the condition and moving the wedging operation out of line. Add an instruction guaranteed to trap after nulling MTCC.

Co-authored-by: David Chisnall <github@theravensnest.org>

Include "Register Atlas"es and updates at key points, making it hopefully easier to understand what's going on at any given point in the code. Annotate arcs in the CFG at their target, not just their source, as well as IRQ disposition and live-in and live-out register sets. The intention is that much of this should be amenable to automatic verification, but no such tooling yet exists. To ease review, this large commit changes no bytes in the actual assembled output. Co-authored-by: David Chisnall <github@theravensnest.org> Co-authored-by: Murali Vijayaraghavan <vmurali@csail.mit.edu> Co-authored-by: Robert Norton <robert.norton@microsoft.com>

As embarrassing as it might be, it's still better to share than not.

nwf requested review from davidchisnall and rmn30 October 18, 2024 23:12

nwf marked this pull request as draft October 18, 2024 23:12

rmn30 reviewed Oct 21, 2024

View reviewed changes

sdk/core/switcher/entry.S Outdated Show resolved Hide resolved

rmn30 reviewed Oct 21, 2024

View reviewed changes

sdk/core/switcher/entry.S Outdated Show resolved Hide resolved

nwf force-pushed the 202410-switcher-commentary branch 5 times, most recently from 37ccd9e to 6616047 Compare October 24, 2024 22:19

nwf changed the title ~~WIP: rework switcher commentary~~ WIP: various tweaks to, and a pile of documentation for, the switcher and exception handler Oct 24, 2024

nwf force-pushed the 202410-switcher-commentary branch 4 times, most recently from e8af222 to ee9a1a6 Compare October 25, 2024 03:49

nwf marked this pull request as ready for review October 25, 2024 03:50

davidchisnall reviewed Oct 25, 2024

View reviewed changes

nwf mentioned this pull request Oct 29, 2024

Switcher fixes from #320 #326

Merged

nwf force-pushed the 202410-switcher-commentary branch 5 times, most recently from 3d0f0df to 76f09b0 Compare October 30, 2024 03:47

nwf mentioned this pull request Oct 31, 2024

switcher: change TrustedStack underflow to exit #328

Merged

nwf force-pushed the 202410-switcher-commentary branch 3 times, most recently from e693d23 to 6a2e33b Compare November 5, 2024 15:38

nwf mentioned this pull request Nov 7, 2024

Use named constants in the switcher #339

Merged

nwf force-pushed the 202410-switcher-commentary branch from 971d800 to 07f8316 Compare November 7, 2024 23:10

This was referenced Nov 11, 2024

IRQs on in compartment fault handlers #345

Merged

scheduler exception path IRQ disposition #346

Merged

nwf force-pushed the 202410-switcher-commentary branch 2 times, most recently from b23dc58 to a3a1b57 Compare November 13, 2024 03:44

nwf force-pushed the 202410-switcher-commentary branch 3 times, most recently from ebc04c8 to 4f9e9f6 Compare November 14, 2024 19:14

nwf and others added 15 commits November 19, 2024 14:34

Fixups from CHERIoT-Platform#343 and CHERIoT-Platform#345

b7dd124

Co-authored-by: David Chisnall <github@theravensnest.org> Co-authored-by: Robert Norton <robert.norton@microsoft.com>

loader: correct stale commentary

f098efc

switcher: rename _skip_compartment_call to _after

d739256

Co-authored-by: Robert Norton <robert.norton@microsoft.com>

switcher: remove some redundant instructions

8f50dc1

Force unwind clobbers a0 and a1; there's no need to be paranoid about their values before possibly trapping or manually invoking force unwind behavior.

switcher: move .Lout_of_trusted_stack towards use

796efaa

.Lout_of_trusted_stack is used only by __Z26compartment_switcher_entryz, so move it up closer to that, next to the other local error paths.

switcher: in args zeroing, move some gets closer to use

bb1bdf9

switcher: small change to register allocation

1851900

When loading the target thread's trusted stack, do more work in csp, just so we don't have to explain that ct0 gets clobbered by something else in the next instruction. This may be excessive.

switcher: avoid reload of mtdc in cross-call

4835788

Having spilled s0 already, we're free to use it rather than clobber t2. We'll zero t2 on the error path here, so there is no concern of it leaking.

switcher: remove useless debug register moves

ba3569b

These were only useful on simulators that did not report all effects of instructions, and we no longer use any such.

switcher: NFC: .Lout_of_trusted_stack zero a1 with the others

296eca6

Co-authored-by: David Chisnall <github@theravensnest.org>

switcher: add scripts/dot_from_switcher.lua

b207877

As embarrassing as it might be, it's still better to share than not.

nwf force-pushed the 202410-switcher-commentary branch from 4f9e9f6 to b207877 Compare November 19, 2024 14:35

rmn30 approved these changes Nov 21, 2024

View reviewed changes

nwf merged commit ebb73af into CHERIoT-Platform:main Nov 21, 2024
6 checks passed

nwf deleted the 202410-switcher-commentary branch November 21, 2024 22:28

nwf mentioned this pull request Nov 22, 2024

Next round of changes to the switcher #358

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: various tweaks to, and a pile of documentation for, the switcher and exception handler #320

WIP: various tweaks to, and a pile of documentation for, the switcher and exception handler #320

nwf commented Oct 18, 2024

nwf commented Oct 25, 2024

davidchisnall left a comment

davidchisnall Oct 25, 2024

nwf Oct 30, 2024

davidchisnall Oct 25, 2024

davidchisnall Oct 25, 2024

nwf Oct 30, 2024

davidchisnall Oct 25, 2024

nwf Oct 30, 2024

davidchisnall Oct 25, 2024

rmn30 Nov 5, 2024

hlef commented Oct 29, 2024

nwf commented Nov 13, 2024

WIP: various tweaks to, and a pile of documentation for, the switcher and exception handler #320

WIP: various tweaks to, and a pile of documentation for, the switcher and exception handler #320

Conversation

nwf commented Oct 18, 2024

nwf commented Oct 25, 2024

davidchisnall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlef commented Oct 29, 2024

nwf commented Nov 13, 2024