search

capablevms / cheri-examples

CHERI sample C programs
Other
10 stars 13 forks source link

Refactor switch_compartment.s #75

Closed gabi-250 closed 2 years ago

gabi-250 commented 2 years ago

This PR contains some changes that helped me grok the code a little better. If you disagree with this refactoring feel free ~to decline this change~ to let me know why. Otherwise, I might follow up with a couple more dedupings.

Since the only real difference between them is the exported switch_compartment_end symbol, it's safe to refactor the 3 examples to use the same switch_compartment.s. Deduplicating the code makes it easier to understand the differences between the secure_* variants.

Some inter_comp_call examples use a correct clean implementation, whereas the secure-update_ddc example uses one that intentionally fails to clear x20. We can conditionally include one of the two implementations in switch_compartment.S to further deduplicate the code.

gabi-250 commented 2 years ago

I think overall, I prefer having the original structure, where each test is within each folder, and changes to files are then emphasized by file suffixes, so one can easily tell where the changes for each test is introduced.

I can rework this to be more to your liking, but i would argue the shared code needs to be refactored out of the examples themselves.

As such, I would argue duplicating this code in completely distinct files helps someone know "oh, there's definitely not anything interesting happening in these files, the changes are in these files which I can put side by side". I think having top-level files with conditional paths makes it harder to see the logic, than just diffing the folders. However, that's perhaps just how I approach things, and it might be better this way, so I won't give a strong decline, but I am partly against it.

De-duplicating the code is meant to save you the trouble of having to manually diff the files. To someone new to the codebase it's not obvious at all which files are interesting and which aren't, which is why I find this version of the code easier to understand (with boilerplate out of the way, I can focus on the parts that are different).

gabi-250 commented 2 years ago

Anyway, I'm not married to this change, so feel free to decline it if you disagree.

ltratt commented 2 years ago

I am generally a fan of factoring out the common stuff and this repo could probably do with a bit of such tidying up!

A question then is: where do we put the common stuff? I don't feel too strongly about that, and I tend to feel that once we've factored out the common stuff, moving it later then becomes easier.

So, personally, I tend to think this PR nudges things in a slightly better direction, even if I'm sure it could be done even better in the future.

0152la commented 2 years ago

Sure, I agree with an overall refactoring of the whole repo (particularly lots of Makefiles lying around and horrible relative paths all over the place, particularly in this section of the repo), so let's get this in as a first step towards that direction then.

0152la commented 2 years ago

One other thing, as indicated by #74 , can we ensure these tests are run in the CI? They might already be, but please double check.

gabi-250 commented 2 years ago

One other thing, as indicated by #74 , can we ensure these tests are run in the CI? They might already be, but please double check.

The inter_comp_call ones already run in CI (I can add the missing ones we talked about in #74, but it's probably best to do that in a separate PR).

gabi-250 commented 2 years ago

A question then is: where do we put the common stuff? I don't feel too strongly about that, and I tend to feel that once we've factored out the common stuff, moving it later then becomes easier.

This PR scatters common stuff (clean.s, clean_leak_ddc.s, switch_compartment.S) in the inter_comp_call directory:

gabi@areca ~/c/c/h/c/inter_comp_call > tree -L 1
.
├── base
├── clean_leak_ddc.s
├── clean.s
├── include
├── secure
├── secure-redirect_clr
├── secure-try_deref
├── secure-update_ddc
└── switch_compartment.S

Perhaps it would be better to move them to a common/ subdir.

Also (and this probably doesn't shouldn't be addressed in this PR), maybe we can sprinkle some e.g. -Iinclude/s in the Makefiles so we don't have to write relative paths when #includeing headers.

ltratt commented 2 years ago

@gabi-250 A quick question based on our in-person conversation: I think you're still making further changes to this PR?

gabi-250 commented 2 years ago

Yep, I've addressed the comments but I haven't pushed yet because I realized the inter_comp_call/base example could also use a similar refactoring (otherwise it'll be the only example left in inter_comp_call that has its own Makefile - the others will be refactored as per Andrei's comment)

gabi-250 commented 2 years ago

This refactoring is proving a bit trickier than I anticipated. I'm debugging an issue (introduced in 2edd256) where the refactored versions of the examples are SIGPROT-ing at an unexpected point, so I don't have anything new ready for review at this point. Anyway, in the interest of transparency I'm going to push some WIP changes (which you are free to ignore).

gabi-250 commented 2 years ago

The SIGPROT issue is fixed in 0d7ed73. The solution is to use adr instead of ldr to load the address of the selected clean function into a register (TIL ldr x13, =label loads the address of label from a literal pool. The indirect load caused a CHERI protection violation because the literal pool was past switch_compartment_end).

gabi-250 commented 2 years ago

The code now works as expected, so it's almost ready for review (it achieves what I described in this comment).

Still to-do:

gabi-250 commented 2 years ago

The code now works as expected, so it's almost ready for review (it achieves what I described in this comment).

Still to-do:

* [ ]  `switch_compartment` takes an additional argument in `x13`, and completely ignores the calling convention. Before merging, I want to update `switch_compartment` to follow the calling convention
* [ ]  the `base` example still has its own Makefile (ideally it should be able to use the top-level one like the other examples)
* [ ]  `switch_compartment.S` should probably be relocated to `common/` subdir or something

As discussed offline, these improvements can be done in separate PRs (in the interest of keeping this one small and easy to review).

gabi-250 commented 2 years ago

I reverted 52e92e5 because a) it probably belongs in a separate PR and b) it doesn't quite work :grinning:.

If you wish, I can squash some of these commits to make the whole thing easier to review.

0152la commented 2 years ago

If you wish, I can squash some of these commits to make the whole thing easier to review.

This PR is indeed getting a bit out of hand. Let's add commits addressing unresolved comment chains, and then decide where to go from there. Personally, I don't understand what happened with https://github.com/capablevms/cheri-examples/pull/75/commits/8f67504a0231e747a113fd3769da81add68329fe, as it seems a lot of files just upped and moved, while the original commit to be reverted had a single file moved.

gabi-250 commented 2 years ago

If I squash it'll be a bit clearer what this PR intends to achieve (that one commit I didn't mean to push and that I later reverted doesn't help either!).

0152la commented 2 years ago

The only concern I have is losing comment chains, but if you think it's worthwhile, feel free to go ahead.

gabi-250 commented 2 years ago

I only want to squash 52e92e5 and the commit that reverts it. Is that alright?

gabi-250 commented 2 years ago

Actually, let me just reset HEAD to 28a439cd159bf6c0b67035f133792fa5a4dec0ae and cherry-pick aaac557e6dbb8ce535d69cbac706d611bad287ec on top (to minimize the number of rewritten commits). Is that cool with you?

0152la commented 2 years ago

The history [1] of the file touched by https://github.com/capablevms/cheri-examples/commit/aaac557e6dbb8ce535d69cbac706d611bad287ec suggests it was affected by the two commits that would be deleted by resetting to https://github.com/capablevms/cheri-examples/commit/28a439cd159bf6c0b67035f133792fa5a4dec0ae. As such, please just reset, and push a new commit, rather than cherry-picking the existing one.

[1] https://github.com/capablevms/cheri-examples/commits/aaac557e6dbb8ce535d69cbac706d611bad287ec/hybrid/compartment_examples/inter_comp_call/build/Makefile.inter_comp_call

gabi-250 commented 2 years ago

As such, please just reset, and push a new commit, rather than cherry-picking the existing one.

What do you mean? Cherry-picking will create a new commit (with a new SHA) with the same contents.

0152la commented 2 years ago

Ok; just do as you think fit and we can see the final result.

gabi-250 commented 2 years ago

Done

0152la commented 2 years ago

Alright. We should take care of the existing comment chains appropriately; if some of them are not applicable anymore due to the history change, please indicate as such.

gabi-250 commented 2 years ago

This PR is indeed getting a bit out of hand. Let's add commits addressing unresolved comment chains, and then decide where to go from there. Personally, I don't understand what happened with 8f67504, as it seems a lot of files just upped and moved, while the original commit to be reverted had a single file moved.

The original commit (52e92e513db4d282cc399103626e0305787d88cc) also moved a lot of files - the revert commit just reverts all those changes. Either way, I've removed both as discussed as they're outside the scope of this PR.

ltratt commented 2 years ago

I think this is now awaiting a review from @0152la, but I might have got confused, and there should be another commit two?

0152la commented 2 years ago

There are some changes I haven't reviewed yet, which I'll go over shortly; unsure if more commits are expected at this stage, but there was an issue with a commit that wasn't meant to be pushed this PR and we decided to revert.

gabi-250 commented 2 years ago

There are some changes I haven't reviewed yet, which I'll go over shortly; unsure if more commits are expected at this stage,

Sorry, I should've been clearer: I have no additional commits to push. I think we need to discuss this comment before I start addressing any of the new comments. TLDR I think the current approach is fundamentally flawed (I happen to prefer code duplication over the version we have in this PR, because I don't think compartments should be able to tell switch_compartment which call implementation to use). From my point of view, we should either

I'm happy with either outcome.

gabi-250 commented 2 years ago

I'm going to decline this PR as it's become rather bloated and hard to review. I'm going to open a number of smaller PR for the minor improvements (typo fixes) we identified in this one, as well as for the refactoring.