Fix switch_compartment PCC bounds
#56
Conversation
|
I'm also working on updating the negative example, as of course it's not as easy as copy pasting, and needs to be debugged... |
|
This looks good to me. @jacobbramley are you able to quickly take a peek? |
|
Also a way of maybe splitting the assembly code in multiple files, so there is less duplication, and less stuff I have to change when I modify core parts. I'll have to play with |
| @@ -63,6 +63,7 @@ extern void comp_f_fn(); | |||
| extern void comp_g_fn(); | |||
| extern void *comp_f_fn_end; | |||
| extern void *comp_g_fn_end; | |||
| extern void *switch_compartment_end; | |||
There was a problem hiding this comment.
The types of these _end variables aren't really correct. These are the types that they point to, which here is just a zero-sized part of the code (.text section). There isn't a good, correct choice, but perhaps representing them as a function pointer is best for this example. Just don't call them; that won't end well. (It shouldn't be possible if the compartment is set up correctly.)
I think this caused your earlier question about taking the address. It's surprising to take the address of a global void* but not so much to take the address of a function, especially given the arithmetic where this is used.
There was a problem hiding this comment.
I was thinking address values are usually represented as void*, but if you think functions are better, I have nothing against that. Amended in 1b46b9c.
There was a problem hiding this comment.
They aren't addresses, though. If anything, they're voids. The label points to the value (which has zero size) and you take the address (&switch_compartment_end) to do some pointer arithmetic.
There was a problem hiding this comment.
I'll maybe have to read more on this, but I believe the current incarnation should be sufficient at the time, until we find a "proper" way (if one such way exists). Or maybe improve how we calculate function sizes in #54 .
There was a problem hiding this comment.
@jacobbramley Are you suggesting using something other than void * e.g. char * or u8*?
There was a problem hiding this comment.
I think @jacobbramley is suggesting s/void */void/.
There was a problem hiding this comment.
I see; I was confused by the small detail. I think what we want is in 420bddc.
There was a problem hiding this comment.
I think you also need to s/extern void *comp_f_fn_end();/extern void comp_f_fn_end();/?
There was a problem hiding this comment.
There was a problem hiding this comment.
Oops, I must have been looking at the wrong thing -- sorry!
| @@ -53,14 +58,11 @@ switch_compartment: | |||
| ldr x11, [x29, x11] | |||
| mov sp, x11 | |||
|
|
|||
| // Derive a new clr to restore PCC, and store it. | |||
| cvtp c11, lr | |||
|
|
|||
| // Install compartment DDC | |||
| msr DDC, c1 | |||
|
|
|||
| // Save old DDC (c2), old SP (x12), old LR (c11) on stack | |||
There was a problem hiding this comment.
The comment (c11) is now out of date.
|
Updated negative example, split files to make things clearer (now the delta between negative and positive example should be clearer in a tree view), and implemented comments. |
|
Please squash. |
A hardcoded value of the bounds over `switch_compartment` meant that the bounds were much wider than expected. This meant two things: - the `clean` function called within `switch_compartment` was reachable and executable; - when deriving the `clr` within `switch_compartment`, we would derive with the larger bound, and return to a compartment with much more executable permissions than intended. We now tightly bind the PCC to `switch_compartment`. This means moving `clean` within these bounds (a better way would be to create capabilities allowing this function to be called, and provide local copies to each compartment, but that is beyond the scope of this example, and an exercise in engineering), and not deriving `clr` within `switch_compartment`, but just retaining the provided `clr`. Additional small cleanups, such as code-base splitting, and comment-fixing.
|
Squashed. |
|
bors r+ |
|
Build succeeded: |
A hardcoded value of the bounds over
switch_compartmentmeant that thebounds were much wider than expected. This meant two things:
cleanfunction called withinswitch_compartmentwas reachableand executable;
clrwithinswitch_compartment, we would derivewith the larger bound, and return to a compartment with much more
executable permissions than intended.
We now tightly bind the PCC to
switch_compartment. This means movingcleanwithin these bounds (a better way would be to createcapabilities allowing this function to be called, and provide local
copies to each compartment, but that is beyond the scope of this
example, and an exercise in engineering), and not deriving
clrwithinswitch_compartment, but just retaining the providedclr.Also some comment inconsistency fixes.