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

C64-Secure ABI demo. #96

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

jacobbramley
Copy link
Contributor

This demo illustrates a few calling-convention options for weak (but potentially fast) compartmentalisation across function calls.

This demo illustrates a few calling-convention options for weak (but
potentially fast) compartmentalisation across function calls.
morello-c64-secure/c64-secure.c Outdated Show resolved Hide resolved
morello-c64-secure/impls.s Outdated Show resolved Hide resolved
@ltratt
Copy link
Collaborator

ltratt commented Apr 19, 2024

@jacobbramley This looks good, and useful -- thanks!

@0152la Is there an easy way of only building this example in CI on Morello? I must admit that I've forgotten how to navigate the infrastructure for this repo...

@jacobbramley
Copy link
Contributor Author

@0152la Is there an easy way of only building this example in CI on Morello? I must admit that I've forgotten how to navigate the infrastructure for this repo...

I think I could add it to .buildbot.sh fairly easily. I'll do that whilst I fix the other issues.

@ltratt
Copy link
Collaborator

ltratt commented Apr 19, 2024

@jacobbramley I remember there's some fiddling because we and try and support multiple platforms. Good luck!

Add the new demo to .buildbot.sh.
Consistently use c10 as a scratch in the 'is_in_A' helper. Also,
briefly document how they work.
Replace `cheri/cheric.h` with `cheriintrin.h`.
Run clang-format.
@0152la
Copy link
Contributor

0152la commented Apr 22, 2024

@0152la Is there an easy way of only building this example in CI on Morello? I must admit that I've forgotten how to navigate the infrastructure for this repo...

I think I could add it to .buildbot.sh fairly easily. I'll do that whilst I fix the other issues.

I think the process is:
(a) to build, it'll need to be added to .buildbot.sh. Currently we seem to build both riscv and morello together [1], so you'll probably need another block that'll be (pseudocode) push; make morello-purecap clean; make morello-purecap all; pop.
(b) to run, add it somewhere in the appropriate block in run_tests.sh [2]. I think since it's built only for morello, it'll only be run for morello, but unsure here.

However, since the infrastructure is a bit flimsy I'd think, perhaps as long as it builds it's fine, or even could preclude adding it. I think there's some stuff that's not been ran in CI, and we should probably add everything once (if) we redo the CI.

[1] https://github.com/capablevms/cheri-examples/blob/master/.buildbot.sh#L16-L25
[2] https://github.com/capablevms/cheri-examples/blob/master/tests/run_tests.sh#L64

.buildbot.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@probablytom probablytom left a comment

Choose a reason for hiding this comment

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

Thanks for the patience with this review being so late. Mostly small notes on comments & explanatory text! Great example.

morello-c64-secure/README.md Show resolved Hide resolved
Comment on lines +130 to +139
// TODO: For now, we just allow 16KB per function, but we could pick the
// longest length that guarantees 16-byte alignment (E=4).
mov x16, #(16 * 1024)
sub x17, sp, x16
scvalue c17, cfp, x17
scbndse c17, c17, x16
sub x16, sp, w0, uxtw #4 // Allocate locals.
scvalue csp, c17, x16
// TODO: The very last section of stack will be unusable because
// scbnds{e} fails if the required bounds exceed the bounds of <Cn>.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these TODOs to be addressed before merge? If not, I wonder whether it'd be clearer for future readers that it's a warning about the prog's behaviour to mark them NB or something similar…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not before merge, since what is implemented is sufficient for the demo, but we might decide to address them in some future update if the corner-cases are interesting.

I normally use "TODO" for such things, but if the project prefers a different marker (like NB), that's fine too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think NB would be clearer! Like you say, what's implemented is fine for the demo itself.

morello-c64-secure/impls.s Outdated Show resolved Hide resolved
Comment on lines 351 to 353
// This should be called from each demo function in a loop. The demo function
// should either call the resulting function, or return (exiting its loop) if
// it's NULL.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is obviously key to the example, and I think clarifying what you mean here would help make this example easy to understand; I think it would have helped me more quickly realise what's going on, which I think is important for an examples repo.

I think it's tricky at the moment because the flow of the example is unusual. When I was first reading the PR, I noticed that main calls Z(0), which then calls this relatively beefy function (compared to main). Because it's large and main is so small, it looks like it could be doing lots of the work the example's demonstrating, but really it's a helper function driving recursive calls to the stuff in the assembly! There's a red herring, but the confusion could be avoided by clarifying here.

So for example this comment could explain this as…:

This is the main interactive logic of the example. Every demo function (found in `impls.s`) calls this to determine how many locals to allocate and which demo function to call next (A, B, C, Z, or . to return). The caller of `what_next` also calls the next demo function requested by the user, so the logic demonstrated by this example can be found there. `what_next` just facilitates the interactive part of the program by allowing the user to input which demo function to call next (and its number of locals).

I realise this is just a comment, but given it's also an example I think clarifying what's going on is valuable for future readers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good suggestion, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworded here: 437b84f

Is that any better?

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks brill! I'd nit-pick the TODO that snuck in for the reasons mentioned in earlier comments (you suggested NB as an alternative in another thread, which I really like) but the reword itself is ace :)

.buildbot.sh Outdated Show resolved Hide resolved
Reorganise .buildbot.sh build stage by target.
Reword the TODO asking for a proof.
Clarify `what_next` comment.
@ltratt
Copy link
Collaborator

ltratt commented Jul 21, 2024

@probablytom Can you mark your comments as resolved (if they're resolved) or comment further? I have the impression this is more-or-less mergeable now, but I'm not 100% sure!

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.

4 participants