Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve IRQ real-time compliance #24

Merged
merged 32 commits into from
Oct 14, 2021
Merged

Improve IRQ real-time compliance #24

merged 32 commits into from
Oct 14, 2021

Conversation

jovanbulck
Copy link
Member

@jovanbulck jovanbulck commented Aug 14, 2020

This PR adds support for interruptible crypto operations (in an abandon-and-restart fashion) + use SSA memory pointer for enclave register content instead of enclave stack.

Still needs to be done before merging:

  • remove UNPROTECTED_IRQ_REG_PUSH from the code base
  • fix failing test cases in run_all_sancus (likely changing the asm stubs to the new SSA convention)
  • update linker to the new SSA convention
  • update trusted runtime (sm_entry.S) to the new SSA convention
  • update sm_support to the new interruptible crypto HW contract
  • disable HW support for confidential loading (not compat w interruptible crypto)

jovanbulck and others added 7 commits July 16, 2020 13:20
Crypto control unit fails the currently executing crypto instruction
(if any) on asynchronous IRQ arrival, setting R2 zero flag to be
able to distinguish IRQ from verification failure. Also includes a
simple mechanism to cancel (free) allocated SM circuit in SM control
unit when interrupting a sancus_enable crypto instruction.
Memory read needs a cycle to stabilize.
@jovanbulck jovanbulck marked this pull request as draft August 14, 2020 11:42
@fritzalder
Copy link
Member

Added sancus-tee/sancus-compiler#33 which add the SSA compatibility for linker and trusted runtime. Those work flawlessly now with maybe 50% of the sancus examples (the rest probably failing because of sm_support and confidential loading)

@jovanbulck
Copy link
Member Author

okay, added some comments on the trusted runtime in the compiler repo. I suspect those may be the cause of the failing tests rather than the interruptible crypto. Normally the sancus-examples don't have a lot of (random) IRQs and confidential loading is still enabled in the core, so I don't expect those to fail the tests.

jovanbulck added a commit to fritzalder/sancus-compiler that referenced this pull request Jan 28, 2021
@jovanbulck
Copy link
Member Author

I think this concludes the changes to sancus-core :-) All todos above have been addressed and all unit tests are normally passing now w/ and w/o the ATOMICITY_MONITOR, but make sure to do a careful review @fritzalder esp for the atomicity monitor changes..

Some notes:

  • the core should normally behave "backwards compatible" before any SM is loaded. Both w/ and w/o ATOMICITY_MONITOR, as also validated by the testsuite.
  • To make sure GIE is respected, I disabled non-maskable interrupts (NMIs) in the openmsp430_defines config file
  • Upon a violation the core always vectors to the registered handler for that IRQ number. The saved status register R2[14] bit can be used later by the interrupted program to learn it suffered a violation. If the violation happens inside an SM in atomic mode, the SSA is completely untouched and all registers still cleared (including r2[14]), else the violation is processed as a normal IRQ (the only difference being R2[14]=1 in the saved SSA frame). This also means that software should be written such that a violation is never expected to happen in (benign) atomic section usage (i.e., don't derefence untrusted pointers in atomic sections). I think this is sufficient and makes sense, atomic sections are already small hand-written asm stubs, and this behavior also corresponds to the "atomic" nature of the code (eg atomic code may want to inspect and change SSA memory w/o having it overwritten).
  • When interrupt-restart crypto is enabled (through ATOMICITY_MONITOR), confidential loading is transparently disabled. Normally any attempt to do sancus_enable with a wrapped SM, will properly indicate verification failure to the software invoking sancus_enable (but I didn't test this explicitly).

Last but not least, I only tested the core under the custom assembly unit test framework. So no guarantees everything still works with the sancus-compiler and this should be checked and ran against sancus-examples. I did push a small commit in sancus-tee/sancus-compiler#33 that should normally add transparent compiler support for interrupt-restart crypto, but again I didn't test this at the level of the compiler/sancus-examples.

@jovanbulck jovanbulck marked this pull request as ready for review January 28, 2021 17:58
@jovanbulck
Copy link
Member Author

jovanbulck commented Sep 23, 2021

@fritzalder why did you need to add an extra wait cycle in the " Adding wait cycle after violations " commit c624d84 above?

Not sure why exactly, but this extra cycle breaks some test cases. Probably a timign issue in the stimulus files, but I would like to make sure the extra wait cycle is really needed before I figure out how to fix the test cases?

@fritzalder
Copy link
Member

The wait cycles are indeed necessary after loading the SSA address. At least in verilator, the read SSA address is otherwise not writeable into the mab in the next cycle which means that on an SM IRQ, the SSA will not be correctly written and the SSA remains empty. Whenever an SM then wants to restore itself from an SSA, it will only read zero bytes and crash.

@jovanbulck
Copy link
Member Author

for reference, after digging deeper with @fritzalder, we established that the wait cycle is needed (at least in verilator simulations) since the result of mdb_in is used in the same cycle for mab when writing the PC to the SSA frame (somehow this doesn't manifest as an issue when using iverilog for the test bench sm_irq).

I'll update the failing unit tests before this commit can be merged.

@fritzalder
Copy link
Member

As discussed, I merged this to speed things up and opened #28 to address the last tests.

@fritzalder fritzalder deleted the irq branch October 14, 2021 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants