Initial design of Morello compartments library #1

0152la · 2022-02-09T15:07:51Z

The functionality is rather limited at the moment, but in the interest of not having too big PRs to review, I think it's a good step to have a sync. Currently, there are two main files of interest:

src/switcher.S, more or less copied from our previous secure [1] example;
src/manager.S, containing new functions for, currently, initialization and adding compartments.
The current system follows the idea that we'll have one function of interest per compartment (although that can easily be changed). In init_compartments, we allocate memory for the switcher, including memory for hosting compartment capabilities which will be used for switching, as well as derive capabilities (DDC/PCC) for the switcher. In add_compartment, we allocate memory for a new compartment, as well as derive appropriate data (DDC) and executable (PCC) capabilities for the compartment and the executable entry point, then store these capabilities within the switcher's memory region (containing comps_addr).

In addition, there are two tests, simple_init and simple_add, which call the above functions, and perform some sanity checks over their functionality.

This will remain a draft until I get bors working with this repo.

[1] https://github.com/capablevms/cheri-examples/tree/master/hybrid/compartment_examples/inter_comp_call/secure

ltratt · 2022-02-09T15:13:33Z

I might have misunderstood this but it looks like there are two global arrays switcher_caps and comps_addr which contain capabilities which any compartment can access at any time?

jacobbramley · 2022-02-09T15:27:49Z

Were they intended to be sealed, perhaps?

0152la · 2022-02-09T15:29:43Z

They will be lying outside the respective DDC of the compartments, so won't be accessible, but yes, I believe these could be sealed. Once I model actual switching here, I will be able to evaluate what is needed precisely. Currently, everything happening here would be in privileged/executive mode.

ltratt · 2022-02-09T15:42:54Z

I am going to assert (which means: I'll be happy if you prove me wrong!) that the current API will always leak capabilities via the arrays. In capablevms/cheri-examples#45 I tried doing something different which is that compartment_create gives you back a (probably sealed?) capability which uniquely identifies a compartment: it's then up to you who you give access to that capability to. I suspect that's the only way to make this API secure(able).

0152la · 2022-02-09T15:50:05Z

Well, those arrays reside in some memory location, let's say 0x1000 to 0x2000. Suppose we construct compartments with memory allocated to them somewhere in regions 0x4000 to 0x40000. Therefore, any derived DDC for those compartments will be constrained within that region, meaning that when we will be executing code within some compartment, we know its DDC will not cover memory region 0x1000 to 0x2000. Do you disagree?

There is a secondary, orthogonal thing I thought about, which is having all executable entry-points within their respective compartments, and have a stub entry point for each compartment, which is called by the switcher, but that's something to be done in the future, I think. That would be closer to what you're proposing in that issue.

ltratt · 2022-02-09T15:52:08Z

Well, those arrays reside in some memory location, let's say 0x1000 to 0x2000. Suppose we construct compartments with memory allocated to them somewhere in regions 0x4000 to 0x40000. Therefore, any derived DDC for those compartments will be constrained within that region, meaning that when we will be executing code within some compartment, we know its DDC will not cover memory region 0x1000 to 0x2000. Do you disagree?

It's not that I disagree per se, but that means that simple_add would presumably have to be rewritten to use a quite different API (e.g. it accesses array elements directly at https://github.com/capablevms/cheri-morello-compartmentalisation/pull/1/files#diff-381d671f972d99b39707fab1f10cb11be8ea79e414a0f5790ea3991be7472ef3)?

0152la · 2022-02-09T15:56:00Z

Well, that would be a different example, if we want to access elements from within a compartment. The two tests do everything from a fully privileged status, as presumably we expect the functionality of creating/deleting compartments to be called from within such a context. To emphasise, the two current tests do not showcase any execution from within compartments, so there are no security guarantees to be checked within these, I don't think.

ltratt · 2022-02-09T16:02:35Z

I'm going to push you to reconsider: I think the array approach is fundamentally broken because either:

We directly leak every compartments capability (as at the moment).
We indirectly leak it by allowing people to say something like "get_capability(1);"

I'm going to go further and (in full awareness of how arrogant this is on my part!) assert that capablevms/cheri-examples#45 has an API which is secure now and will remain so in the future.

Warning: I might be wrong!

0152la · 2022-02-09T16:05:39Z

I disagree, but what I suggest we do is that we wait until I add a test which spawns more compartments, and actually does inter-compartment calls (which I'm unsure precisely how it'll look like at the moment) and then we can model attempts at leaking these stored capabilities. Is that agreeable?

ltratt · 2022-02-09T16:06:46Z

Before you do that, I think you need to prove that compartment A can never get access to compartment B's capability (unless someone explicitly gives A such a capability). If you can do that, then I'm willing to let go.

ltratt · 2022-02-09T16:30:34Z

After an offline chat, I had misunderstood that the examples are (in essence) running "inside" the (privileged) compartment switcher. My comments above thus don't make fully sense. Mea culpa!

0152la · 2022-02-09T16:31:51Z

bors try

0152la · 2022-02-09T16:58:19Z

bors try-

0152la · 2022-02-09T17:00:36Z

bors ping

bors · 2022-02-09T17:00:37Z

pong

0152la · 2022-02-09T17:04:21Z

bors try

0152la · 2022-02-10T11:05:49Z

bors try

bors · 2022-02-10T11:05:50Z

try

Already running a review

0152la · 2022-02-10T11:06:01Z

bors try-

0152la · 2022-02-10T11:06:15Z

bors try

bors · 2022-02-10T11:06:21Z

try

Build failed:

buildbot/capablevms-test-script

0152la · 2022-02-10T11:22:01Z

bors try

bors · 2022-02-10T11:22:07Z

try

Build failed:

buildbot/capablevms-test-script

0152la · 2022-02-10T11:24:52Z

bors try

0152la · 2022-02-11T08:08:51Z

Squashed the boilerplate commits away.

ltratt · 2022-02-11T08:11:53Z

COPYRIGHT

+The following contributors wish to explicitly make it known that the copyright
+of their contributions is retained by an organisation:
+
+    Jacob Bramley <[email protected]>:


I think (offline) Jacob was right to say that people committing from an institutional address don't need to be listed here. It's just awkward people like me!

It's true, such contributors do not need to be explicitly listed, but it's also harmless to do so. I have no objection either way. When I made my earlier (offline) comments, I was confused about the nature of this file, otherwise I wouldn't have said anything.

ltratt · 2022-02-11T08:12:33Z

I will be fixing the solution I've chosen, as I can't be bothered to find a "proper" way of doing this.

I'm not sure I understand this. Can you expand please?

0152la · 2022-02-11T08:23:50Z

To give a bit of background, before I started integrating bors in this project, I already was having a working-state testing infrastructure. Ideally, I'd be able to just apply that existing infrastructure with no changes, in the testing environment (or update it in case things are improperly engineered). However, it seems I need to add these StrictHostKeyChecking no options to commands in my test script [1], just to get it working on our buildbot worker container. I dislike this solution of having to modify the generic testing infrastructure, but after having spent a few hours getting annoyed at buildbot, I just decided to "make it work", so we can make progress with the PR.

[1] https://github.com/capablevms/cheri-morello-compartmentalisation/pull/1/files#diff-a846d9efb204234acca8da8ca8a4ad031932989e1da0ef08960fd78ec3d60fa2R16-R17

ltratt · 2022-02-11T08:26:33Z

I'm confused: are you saying the testing infrastructure for (say) cheri-examples doesn't work here or ... ?

0152la · 2022-02-11T08:31:43Z

There are three components to using our buildbot infrastructure:

bors/buildbot communication
spawning a QEMU CHERI instance
running tests for the project

The first two, I've reused from cheri-examples. The last, of course, should be project specific, and by testing infrastructure above, I meant the testing of this project, by its own. In order to move that in our buildbot setup, I had to modify it slightly to make it compatible, when I think those requested changes should be done on the buildbot side (or understand if there is a better solution, since I don't fully understand the core cause), but that is another conversation.

ltratt · 2022-02-11T08:33:21Z

Ah I see! My guess is that the "running tests for the project" thing is necessarily repo specific (even if it's often very similar from repo to repo). Might be worth discussing with @drpdr in case I'm wrong though!

Please squash the PR overall as you see fit.

0152la · 2022-02-11T08:37:27Z

I've been discussing with Pino, and proposed something, but we need to think it through first.

By squashing, you mean this is still too many commits? I've squashed everything after 96a72ee, as per your above comment. I can squash the ones before, as well, to just have a monolithic commit with all the implementation stuff.

ltratt · 2022-02-11T08:40:32Z

You should squash as you see fit, but yes please squash things back to the beginning of the PR so that I can merge this. That doesn't mean you have to squash all the commits away (but e.g. I doubt 20 commits in history is useful for what this PR does! What the right number is, though, I leave up to you).

* usual `src`, `include`, `tests` setup * use in-built cmake option for testing * sketch out `init_compartments` and `add_compartment` functions

As discussed, we move towards a more abstract model, including memory management in the assembly layer. This updates the initialiation function to not require any user input, and setup memory appropriately, for future use. Add checks to `simple_init` to sanity check functionality.

0152la · 2022-02-11T08:57:27Z

I think this should be a bit cleaner, while maintaining a high-level overview.

ltratt · 2022-02-11T09:06:13Z

src/manager.S

+    add       x4, x4, #1
+    str       x4, [x3]
+
+    // Update switcher DDC


Is there a reason this is commented?

Ah right. Initially I was thinking if the switcher DDC should be dynamic, meaning that as we create compartments, therefore associated capabilities, we lengthen the visible space for the switcher (and vice-versa for deleting). But then I thought that we allocate some maximum memory at the start anyway (and currently there is no handling for what happens if we need more memory than that max), so wouldn't dynamically changing the DDC just add complexity, without much gain?

Good point. In the longer term, we will need to allow the switcher to create (and delete!) an arbitrary number of compartments. However, that shouldn't change the external API, so it's fine if we only allow a fixed number at first (but if we do that, we should add a check so that we abort the process if the user tries to exceed that fixed number).

Implemented in 61b9d8a.

Looks good to me -- but can we also delete the commented out code if it's not needed?

Done in 7e16316 for now. I guess we can keep the idea in mind, if we do want to take that approach.

Lots of new code seems to have crept in with 7e16316 ?

Fixed in 3defc97. (that was painful)

ltratt · 2022-02-11T14:20:49Z

Please squash.

0152la · 2022-02-11T15:03:54Z

I suggest we first wait for @jacobbramley to give us some feedback, once he's back from his holiday. I'll keep working on my fork, so this not being merged won't be a blocker.

jacobbramley · 2022-02-17T14:56:16Z

.gitignore

@@ -0,0 +1,6 @@
+build/
+
+**/*.swp


Is that for Vim swap files? Isn't it better to ignore those in a user ~/.gitignore (or similar)?

I like having robust per-project configs, so when I work with them remotely, the settings are maintained. Are you suggesting we remove the swap files here by any chance?

I was, but it's pretty clearly bikeshedding so please disregard :-)

jacobbramley · 2022-02-17T15:02:57Z

CMakeLists.txt

@@ -0,0 +1,24 @@
+cmake_minimum_required(VERSION 3.16)


We dropped cmake for cheri-examples. I don't remember all the reasoning (other than that it just wasn't required). Is it necessary here? I have no particular opinion one way or the other, except that simple is good.

Personally, I understand cmake better than make, so I prefer it. If there is desire to have make consistently, I have no strong feelings either way, although do prefer cmake for myself.

jacobbramley · 2022-02-17T15:05:15Z

COPYRIGHT

+The following contributors wish to explicitly make it known that the copyright
+of their contributions is retained by an organisation:
+
+    Jacob Bramley <[email protected]>:


It's true, such contributors do not need to be explicitly listed, but it's also harmless to do so. I have no objection either way. When I made my earlier (offline) comments, I was confused about the nature of this file, otherwise I wouldn't have said anything.

jacobbramley · 2022-02-17T15:16:06Z

src/manager.S

+asm_call_wrapper:
+    // Save `x0` so we can temporarily use it
+    cvtp      c0, x0
+    str       c0, [sp]


This will overwrite the caller's stack. Storing to [sp] is a poke, not a push. The processor won't complain — AAPCS64 is just a software convention and we're already deviating from it — but I'm fairly sure that it's not what you want.

str c0, [sp, #-16]! // Push-like behaviour. ldr c0, [sp], #16 // Pop-like behaviour.

Corresponding sequences elsewhere in the patch use the instructions that I would expect, so this appears to be an isolated bug (or misunderstanding on my part).

On AArch64 we often coalesce multiple pushes or pops to avoid repeatedly updating sp. I wrote about this in a blog; nothing fundamentally changes with Morello, except that you can push individual (16-byte) C registers, which simplifies things sometimes.

Also, see my next comment :-)

Right, I've done this in my other functions, but haven't managed to fix this up.

jacobbramley · 2022-02-17T15:35:03Z

src/manager.S

+    cvtp      c0, x0
+    str       c0, [sp]
+
+    // Derive `clr` (in case asm function does something weird with `PCC`)


Ok, that's a very useful comment, because my first thought otherwise is "why are you preserving PCC metadata and not just lr?" Thanks!

Some observations:

You don't need to do this for both c0 and lr. It's not harmful, but might be clearer if you do it only for lr (since you can only restore PCC from one anyway), and then there's an obvious free stack slot for something else, should you need it later.

I'm 99% sure that you don't want to use swp, which does an atomic swap. In this case, it will swap c0 (containing the lr with the PCC's metadata) with [sp] (containing the target function pointer with the PCC's metadata). However, assuming no other threads are fiddling with the same memory, you had the target function pointer in a register a few instructions ago anyway.

cvtp explicitly derives a capability from PCC. If you need to save or handle a function pointer, that's what you want. However, if you're only going to branch to it (like blr c0) then you might as well just branch to the X register (blr x0).

Taking those points into account, you probably want something this shape:

asm_call_wrapper: cvtp clr, lr str clr, [sp, #-32]! str x0, [sp, #16] // Stack layout here: [ unused ] // [ x0 (target fn) ] // [ clr (high64) ] // sp -> [ clr (low64) ] blr x0 // New PCC is implicitly derived from current PCC, using the address in x0. ldr clr, [sp], #32 ret clr

That's obviously untested. Use at your own risk, etc.

So the reasoning behind this function was to make my life easier, not needing to save some registers as I'm coming from C-world into asm, and just use this to call asm functions from C. I think it made it harder, as not only would the call point look bad (couldn't call an asm function directly, needed to go through this function), I think setting the registers correctly to the inner called functions is just a waste of resources. So I'll just get rid of it for now [1]. The asm functions are already able to be called directly.

The note about cvtp and blr is useful. It makes sense, I just hadn't thought of it before. Something to keep in mind.

[1] 13a35e5

jacobbramley · 2022-02-17T17:26:55Z

src/switcher.S

+    mov      x0, #0
+    cvtp     clr, lr
+    b        switch_compartment
+    ret      clr


Do we ever come back here? The b looks like a tail call, so is this instruction necessary?

Ah, good point. We can just drop through. I copy pasted this code from the previous switcher iteration (and honestly shouldn't have included it in this PR), but for completeness, I updated it in 08f834f.

jacobbramley · 2022-02-17T17:27:36Z

src/switcher.S

+
+/** Code to perform actual switch
+ *
+ * @param x0 ID (in `comps` array) of compartment to switch to


It also expects the switcher DDC in c29, and it assumes that its address is set to some data area too. (The DDC address is usually ignored.)

Yes, I've documented this in the heavily modified local copy I'm working on right now. Thanks for spotting that.

jacobbramley · 2022-02-17T17:28:30Z

src/switcher.S

+ */
+.type switcher_entry, "function"
+switcher_entry:
+    mov      c29, c0


Note that c29 is callee-saved, so this should preserve it (since this is called from C code).

A simple fix for now would be to pass it in some caller-saved register not used for arguments (e.g. c10). However, I think we picked c29 earlier because it's also special for ldrblr etc. Currently, we aren't using that, but maybe there's a development plan for it.

That's a good point, having to save c29. Let's leave this conversation for the PR where the switcher is actually used. I'm sure there's more correctness issues to be discussed then.

I've no objection in principle, but I worry that it'll be forgotten and bite us later. Bugs like these tend to go unnoticed because simple calling code might continue to work anyway.

The safer approach would be s/c29/c10/ now (here and in switch_compartment). If we want to use ldpblr later, we can revisit this.

I can do this for completion here, but the problem is I'd be needing to rebase those changes into my local branches, which are complete (and include ldpblr use appropriately). If you think it's worth it for having this PR more "complete", then I can do that, but this code is due to be overwritten shortly anyway. And I know I cut some corners regarding saving registers in the implementation I have locally, so that's something I definitely want to fix over in that PR; I definitely won't let it go.

I'm not insisting on anything, so handle this as you prefer :-)

jacobbramley · 2022-02-17T17:36:58Z

src/switcher.S

+    // Save old DDC (c2), old SP (x12), old CLR (clr) on stack
+    stp       c2, clr, [sp, #-48]!
+    str       x12, [sp, #32]


Doesn't this make them reachable from the callee compartment? If so, why isn't it a security hole?

Aha, we found a security hole in what we considered a secure implementation of switching [1]. Indeed, these should be saved before we swap the DDC. I will keep this in mind as I work on the switcher. We could probably add a test which shows this can be done on the prior example, then fix the security hole.

[1] https://github.com/capablevms/cheri-examples/blob/master/hybrid/compartment_examples/inter_comp_call/secure/switch_compartment.s#L35-L40

src/switcher.S

0152la · 2022-02-23T13:45:40Z

I think the discussion around this PR has reached a fixed point, and we can go ahead and merge it.

ltratt · 2022-02-23T13:45:53Z

Please squash.

* Add checks to ensure we don't add more compartments than we allocated space for * Small optimisations and unneeded code removal Remove comment Simplify Replace multiple instruction with a single one Remove unimplemented stub Optimization Better register usage Fix Fix Forgot to update checked register Removed unused function Remove an unneeded call Remove unneeded instruction

0152la · 2022-02-23T13:51:20Z

Squashed.

ltratt · 2022-02-24T18:12:41Z

bors r+

bors · 2022-02-24T18:17:14Z

Build succeeded:

buildbot/capablevms-test-script

0152la assigned ltratt and jacobbramley Feb 9, 2022

bors bot added a commit that referenced this pull request Feb 9, 2022

Try #1:

8fe46bf

bors bot added a commit that referenced this pull request Feb 9, 2022

Try #1:

b497996

bors bot added a commit that referenced this pull request Feb 10, 2022

Try #1:

fbf7ae3

bors bot added a commit that referenced this pull request Feb 10, 2022

Try #1:

cbb95c4

0152la marked this pull request as ready for review February 11, 2022 08:08

ltratt reviewed Feb 11, 2022

View reviewed changes

0152la added 4 commits February 11, 2022 08:54

Add basic infrastructure, and initial work

9c4277c

* usual `src`, `include`, `tests` setup * use in-built cmake option for testing * sketch out `init_compartments` and `add_compartment` functions

Finalize simple_add test

9f4758c

Add bors + buildbot support, and legal stuff

6a2c543

0152la force-pushed the init-and-add2 branch from 4fe61d2 to 6a2c543 Compare February 11, 2022 08:56

ltratt reviewed Feb 11, 2022

View reviewed changes

0152la force-pushed the init-and-add2 branch from 7e16316 to 3defc97 Compare February 11, 2022 12:11

jacobbramley reviewed Feb 17, 2022

View reviewed changes

0152la mentioned this pull request Feb 18, 2022

Potential security flaw in "secure" model capablevms/cheri-examples#60

Open

0152la force-pushed the init-and-add2 branch from 80e78ca to c804e09 Compare February 23, 2022 13:51

bors bot merged commit 403ad61 into capablevms:master Feb 24, 2022

		@@ -0,0 +1,6 @@
		build/

		*/.swp

Initial design of Morello compartments library #1

Initial design of Morello compartments library #1

Conversation

0152la commented Feb 9, 2022

ltratt commented Feb 9, 2022

jacobbramley commented Feb 9, 2022

0152la commented Feb 9, 2022

ltratt commented Feb 9, 2022

0152la commented Feb 9, 2022

ltratt commented Feb 9, 2022

0152la commented Feb 9, 2022

ltratt commented Feb 9, 2022

0152la commented Feb 9, 2022

ltratt commented Feb 9, 2022

ltratt commented Feb 9, 2022

0152la commented Feb 9, 2022

0152la commented Feb 9, 2022

0152la commented Feb 9, 2022

bors bot commented Feb 9, 2022

0152la commented Feb 9, 2022

0152la commented Feb 10, 2022

bors bot commented Feb 10, 2022

try

0152la commented Feb 10, 2022

0152la commented Feb 10, 2022

bors bot commented Feb 10, 2022

try

0152la commented Feb 10, 2022

bors bot commented Feb 10, 2022

try

0152la commented Feb 10, 2022

0152la commented Feb 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltratt commented Feb 11, 2022

0152la commented Feb 11, 2022

ltratt commented Feb 11, 2022

0152la commented Feb 11, 2022 • edited Loading

ltratt commented Feb 11, 2022

0152la commented Feb 11, 2022

ltratt commented Feb 11, 2022

0152la commented Feb 11, 2022

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

ltratt commented Feb 11, 2022

0152la commented Feb 11, 2022

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

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

Choose a reason for hiding this comment

0152la commented Feb 23, 2022

ltratt commented Feb 23, 2022

0152la commented Feb 23, 2022

ltratt commented Feb 24, 2022

bors bot commented Feb 24, 2022

0152la commented Feb 11, 2022 •

edited

Loading