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

switcher: change TrustedStack underflow to exit #328

Merged
merged 5 commits into from
Nov 5, 2024

Conversation

nwf
Copy link
Member

@nwf nwf commented Oct 30, 2024

Rather than infinitely looping such a damaged thread, request that the scheduler stop running it (via .Lset_mcause_and_exit_thread). This should never happen, as .Lpop_trusted_stack_frame tries to exit any thread that runs out of trusted stack frames, but the scheduler could ask us to resume such a thing anyway.

@davidchisnall
Copy link
Collaborator

If we're handling this path, maybe we should initialise cra in new threads to the switcher's return entry point so that return from an entry point is a graceful thread exit?

@nwf
Copy link
Member Author

nwf commented Oct 30, 2024

Ah interesting, a use case for manually csealing return sentries! In that case, we'll want .Lskip_compartment_call to not be so .L-ocal so we can grab it in the loader...

I'll whip something up.

@nwf nwf force-pushed the 202410-switcher-exit_again branch from 0409f59 to 4385a7a Compare October 30, 2024 15:13
@nwf nwf requested a review from rmn30 October 30, 2024 15:14
@nwf
Copy link
Member Author

nwf commented Oct 30, 2024

CI failures (so far; there may be others lurking) look to be due to CHERIoT-SAFE's Ibex submodule not including microsoft/cheriot-ibex@439b27ce ; Kunyan is looking at what I'd thought would be an easy PR, microsoft/cheriot-safe#18 .

@nwf nwf force-pushed the 202410-switcher-exit_again branch from 4385a7a to 46db994 Compare October 30, 2024 16:43
sdk/core/loader/boot.cc Outdated Show resolved Hide resolved
Copy link
Collaborator

@rmn30 rmn30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once CI is happy. I like that this cleans up the thread exit path and doesn't even introduce a new untrusted entry point into the switcher.

@nwf nwf force-pushed the 202410-switcher-exit_again branch from 3956116 to 71ceb67 Compare October 31, 2024 17:46
@davidchisnall davidchisnall force-pushed the 202410-switcher-exit_again branch from 71ceb67 to b57561d Compare November 5, 2024 09:14
nwf added 2 commits November 5, 2024 15:40
Rather than infinitely looping such a damaged thread, request that the
scheduler stop running it (via .Lset_mcause_and_exit_thread).  This
should never happen, as .Lpop_trusted_stack_frame tries to exit any
thread that runs out of trusted stack frames, but the scheduler could
ask us to resume such a thing anyway.
@nwf nwf force-pushed the 202410-switcher-exit_again branch from b57561d to 13dbb90 Compare November 5, 2024 15:51
sdk/core/loader/boot.cc Outdated Show resolved Hide resolved
sdk/firmware.ldscript.in Show resolved Hide resolved
Comment on lines +347 to +352
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wc99-designator"
constexpr SealingType Sentries[] = {
[int(InterruptStatus::Enabled)] = ReturnSentryEnabling,
[int(InterruptStatus::Disabled)] = ReturnSentryDisabling};
#pragma clang diagnostic pop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me a bit nervous. I'd normally do this kind of compile-time map with a template so that we get a compile error if something is invalid.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just following seal_entry immediately above. I'd be happy to rework both however desired, tho'.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, my code above is also bad. I think we want something like:

template<InterruptStatus T>
constexpr SealingType SealingTypeForInterruptStatus;

template<>
constexpr SealingType SealingTypeForInterruptStatus<InterruptStatus::Enabled> = ReturnSentryEnabling;

...

Assuming I got the syntax right.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, no, this needs to be a dynamic thing. Never mind. Leave as is for now.

nwf and others added 3 commits November 5, 2024 17:06
Add its offset to the image header and change the header magic number.
If a thread returns from its topmost compartment, it should now directly
enter the thread exit path without first going through a trap on an
untagged cra.
Now that the initial cra for threads is a sentry to a graceful exit and
not nullptr, there's no need to detect and notch out exceptions arising
from thread exits.

Co-authored-by: Robert Norton <robert.norton@microsoft.com>
@nwf nwf force-pushed the 202410-switcher-exit_again branch from 13dbb90 to 0dfbced Compare November 5, 2024 17:07
@nwf nwf merged commit 76a7a27 into CHERIoT-Platform:main Nov 5, 2024
6 checks passed
@nwf nwf deleted the 202410-switcher-exit_again branch November 5, 2024 18:00
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.

3 participants