-
Notifications
You must be signed in to change notification settings - Fork 1
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
Initial design of Morello compartments library #1
Changes from 4 commits
9c4277c
4c26fa2
9f4758c
6a2c543
c804e09
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
#!/bin/bash | ||
|
||
set -e | ||
|
||
build_dir="$(pwd)/build" | ||
src_dir="$(pwd)" | ||
cheri_dir="/home/buildbot/cheri/output" | ||
|
||
# Build project locally | ||
export CC=$cheri_dir/morello-sdk/bin/clang | ||
export CFLAGS="--config cheribsd-morello-hybrid.cfg" | ||
export ASMFLAGS="--config cheribsd-morello-hybrid.cfg" | ||
|
||
cmake \ | ||
-G Ninja \ | ||
-DCMAKE_BUILD_TYPE=DEBUG \ | ||
-DLLVM_DIR=$cheri_dir/morello-sdk/lib/cmake/llvm \ | ||
-DClang_DIR=$cheri_dir/morello-sdk/lib/cmake/clang \ | ||
-B $build_dir \ | ||
-S $src_dir | ||
cmake --build $build_dir | ||
|
||
# Set arguments for Morello hybrid instance | ||
export SSHPORT=10086 | ||
export PYTHONPATH="/home/buildbot/build/test-scripts" | ||
|
||
args=( | ||
--architecture morello-hybrid | ||
# Qemu System to use | ||
--qemu-cmd $cheri_dir/morello-sdk/bin/qemu-system-morello | ||
--bios edk2-aarch64-code.fd | ||
--disk-image $cheri_dir/cheribsd-morello-hybrid.img | ||
# Required build-dir in CheriBSD | ||
--build-dir . | ||
--ssh-port $SSHPORT | ||
--ssh-key $HOME/.ssh/id_ed25519.pub | ||
) | ||
|
||
# Spawn CHERI QEMU instance, then execute `ctest` | ||
python3 $src_dir/.run_cheri_qemu_and_test.py "${args[@]}" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
build/ | ||
|
||
**/*.swp | ||
|
||
build.sh | ||
up.sh |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
#!/usr/bin/env python3 | ||
|
||
# Adapted from | ||
# https://github.com/capablevms/cheri-examples/blob/master/tests/run_cheri_examples.py | ||
|
||
import argparse | ||
import importlib.util | ||
import os | ||
import subprocess | ||
import sys | ||
|
||
from pathlib import Path | ||
|
||
# Emulate `sys.path` from path of module `run_tests_common` (found via | ||
# environment variable `PYTHONPATH`), as required by the CHERI testing | ||
# infrastructure which we are using to simplify booting a QEMU instance | ||
test_scripts_dir = str(Path(importlib.util.find_spec("run_tests_common").origin).parent.absolute()) | ||
sys.path = sys.path[sys.path.index(test_scripts_dir):] | ||
|
||
from run_tests_common import boot_cheribsd, run_tests_main | ||
|
||
def run_tests(qemu: boot_cheribsd.QemuCheriBSDInstance, args: argparse.Namespace) -> bool: | ||
if args.sysroot_dir is not None: | ||
boot_cheribsd.set_ld_library_path_with_sysroot(qemu) | ||
boot_cheribsd.info("Running tests for cheri-morello-compartmentalisation") | ||
|
||
# Run command on host to test the executed client | ||
os.chdir(f"{args.build_dir}/build") | ||
subprocess.run(["/usr/bin/ctest", "-V"], check=True) | ||
return True | ||
|
||
if __name__ == '__main__': | ||
# This call has the side-effect of booting a QEMU instance | ||
run_tests_main(test_function=run_tests, need_ssh=True, should_mount_builddir=False) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
cmake_minimum_required(VERSION 3.16) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We dropped cmake for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally, I understand |
||
project(CheriMorelloCompartments LANGUAGES C ASM) | ||
|
||
set(INCLUDE_DIR ${CMAKE_SOURCE_DIR}/include) | ||
set(SRC_DIR ${CMAKE_SOURCE_DIR}/src) | ||
set(TEST_DIR ${CMAKE_SOURCE_DIR}/tests) | ||
|
||
# Setup | ||
include_directories("${INCLUDE_DIR}") | ||
|
||
find_package(Clang REQUIRED) | ||
find_package(LLVM REQUIRED CONFIG) | ||
|
||
# Building | ||
add_subdirectory(${SRC_DIR}) | ||
|
||
# Testing | ||
if (CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME) | ||
include(CTest) | ||
endif() | ||
|
||
if (CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME AND BUILD_TESTING) | ||
add_subdirectory(${TEST_DIR}) | ||
endif() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
Except as otherwise noted (below and/or in individual files), this project is | ||
licensed under the Apache License, Version 2.0 <LICENSE-APACHE> | ||
<http://www.apache.org/licenses/LICENSE-2.0> or the MIT license <LICENSE-MIT> | ||
<http://opensource.org/licenses/MIT>, at your option. | ||
|
||
Copyright is retained by contributors and/or the organisations they | ||
represent(ed) -- this project does not require copyright assignment. Please see | ||
version control history for a full list of contributors. Note that some files | ||
may include explicit copyright and/or licensing notices. | ||
|
||
The following contributors wish to explicitly make it known that the copyright | ||
of their contributions is retained by an organisation: | ||
|
||
Jacob Bramley <[email protected]>: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
copyright retained by Arm Limited | ||
Laurence Tratt <[email protected]>: | ||
copyright retained by King's College London |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
Licensed under the Apache License, Version 2.0 (the "License"); you may not use | ||
this file except in compliance with the License. You may obtain a copy of the | ||
License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software distributed | ||
under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR | ||
CONDITIONS OF ANY KIND, either express or implied. See the License for the | ||
specific language governing permissions and limitations under the License. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
Permission is hereby granted, free of charge, to any person obtaining a copy of | ||
this software and associated documentation files (the "Software"), to deal in | ||
the Software without restriction, including without limitation the rights to | ||
use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies | ||
of the Software, and to permit persons to whom the Software is furnished to do | ||
so, subject to the following conditions: | ||
|
||
The above copyright notice and this permission notice shall be included in all | ||
copies or substantial portions of the Software. | ||
|
||
THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
SOFTWARE. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
# Sourced from | ||
# https://github.com/capablevms/cheri-examples/blob/master/bors.toml | ||
status = ["buildbot/capablevms-test-script"] | ||
|
||
timeout_sec = 600 # 10 minutes | ||
|
||
# Have bors delete auto-merged branches | ||
delete_merged_branches = true | ||
|
||
cut_body_after = "" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
#define COMP_SIZE 48 | ||
#define COMP_OFFSET_PCC 0 | ||
#define COMP_OFFSET_DDC 16 | ||
#define COMP_OFFSET_STK_ADDR 32 | ||
|
||
#define MAX_COMP_COUNT 2 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
add_library(morello-compartments STATIC | ||
${SRC_DIR}/manager.S | ||
${SRC_DIR}/switcher.S | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,169 @@ | ||
.data | ||
|
||
comps_cnt: .dword 0 | ||
comps_addr: .dword 0 | ||
switcher_caps: .dword 0 | ||
|
||
.text | ||
.balign 4 | ||
|
||
#include "comps_offsets.h" | ||
|
||
// Variables | ||
.global comps_cnt | ||
.global comps_addr | ||
.global switcher_caps | ||
|
||
// Functions | ||
.global asm_call_wrapper | ||
.global init_compartments | ||
.global add_compartment | ||
.global del_compartment | ||
|
||
/** | ||
* Wrapper to store/restore state when coming from C | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What this really does is call Note, however, that this isn't a security guarantee: the callee (at There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm having a hard time seeing on the github interface what you are referring to, so I'll assume it's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's not used, then it's probably best to drop it, if only to simplify the analysis. (I hadn't spotted that it's not used.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function |
||
* | ||
* @param x0 ASM function to call | ||
* @param x1-x6 parameters to pass to function in c0 | ||
*/ | ||
.type asm_call_wrapper, "function" | ||
asm_call_wrapper: | ||
// Save `x0` so we can temporarily use it | ||
cvtp c0, x0 | ||
str c0, [sp] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will overwrite the caller's stack. Storing to
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 Also, see my next comment :-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I've done this in my other functions, but haven't managed to fix this up. |
||
|
||
// Derive `clr` (in case asm function does something weird with `PCC`) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, that's a very useful comment, because my first thought otherwise is "why are you preserving PCC metadata and not just Some observations:
Taking those points into account, you probably want something this shape:
That's obviously untested. Use at your own risk, etc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 [1] 13a35e5 |
||
cvtp c0, lr | ||
swp c0, c0, [sp] | ||
sub sp, sp, #16 | ||
|
||
blr c0 | ||
ldr clr, [sp, #16]! | ||
ret clr | ||
|
||
/** | ||
* Sets up memory for compartments | ||
* | ||
* Initializes required memory for compartments. This involves allocating a | ||
* memory region for use by switcher code, to contain required capabilities, | ||
* and deriving appropriate PCC/DDC values containing the executable switcher | ||
* code, and the aforementioned memory region, respectively. | ||
* | ||
* @return Pointer to newly allocated memory region | ||
*/ | ||
.type init_compartments, "function" | ||
init_compartments: | ||
// Compute size of required memory, equivalent to `length` parameter of | ||
// `mmap` | ||
mov x0, #COMP_SIZE | ||
mov x1, #MAX_COMP_COUNT | ||
mov x2, #32 // size of the 2 switcher capabilities | ||
madd x1, x0, x1, x2 | ||
jacobbramley marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Store length and `lr` on stack, as we'll need them later | ||
stp x1, lr, [sp, #-16]! | ||
|
||
// Allocate memory for switcher | ||
mov x0, xzr // address | ||
// length - already stored in `x1` | ||
mov w2, #3 // prot == PROT_READ | PROT_WRITE | ||
mov w3, #4098 // flags == MAP_PRIVATE | MAP_ANONYMOUS | ||
mov w4, #-1 // fd | ||
mov w5, wzr // offset | ||
bl mmap | ||
|
||
// Restore length and `lr` | ||
ldp x1, lr, [sp], #16 | ||
|
||
// Save pointer to new allocated memory in `switcher_caps` | ||
adr x2, switcher_caps | ||
str x0, [x2] | ||
|
||
// Derive DDC for switcher | ||
cvtp c2, x0 | ||
scbnds c2, c2, x1 | ||
|
||
// Derive PCC for `switch_compartments` and friends | ||
adr x3, switcher_entry | ||
adr x4, switch_compartment_end | ||
sub x4, x4, x3 | ||
cvtp c3, x3 | ||
scbndse c3, c3, x4 | ||
|
||
// Store (DDC, PCC) at `switcher_caps` | ||
ldr x1, switcher_caps | ||
stp c2, c3, [x1], #32 | ||
jacobbramley marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Save start address for compartment capabilities in `comps_addr` | ||
adr x2, comps_addr | ||
str x1, [x2] | ||
|
||
ret | ||
|
||
/** | ||
* Function to add information for a compartment | ||
* | ||
* @param x0 Compartment memory size | ||
* @param x1 Compartment executable function | ||
* | ||
* @return Pointer to newly allocated memory region | ||
*/ | ||
.type add_compartment, "function" | ||
add_compartment: | ||
// Store inputs and `lr` so we can call `mmap` | ||
stp x0, x1, [sp, #-32]! | ||
str lr, [sp, #16] | ||
mov x1, x0 | ||
|
||
// Allocate memory for new compartment | ||
mov x0, xzr // address | ||
// length - already stored in `x1` | ||
mov w2, #3 // prot == PROT_READ | PROT_WRITE | ||
mov w3, #4098 // flags == MAP_PRIVATE | MAP_ANONYMOUS | ||
mov w4, #-1 // fd | ||
mov w5, wzr // offset | ||
bl mmap | ||
|
||
// Restore memory size and function address | ||
ldp x1, x2, [sp], #32 | ||
ldr lr, [sp, #-16] | ||
|
||
// Derive compartment DDC | ||
cvtp c0, x0 | ||
scbnds c0, c0, x1 | ||
|
||
// Derive compartment PCC | ||
cvtp c1, x2 | ||
mov x2, #320 // TODO dynamic value | ||
scbndse c1, c1, x2 | ||
|
||
// Store new PCC and DDC | ||
ldr x2, comps_addr | ||
ldr x3, comps_cnt | ||
mov x4, #COMP_SIZE | ||
madd x2, x3, x4, x2 | ||
stp c0, c1, [x2] | ||
|
||
// Increment counter | ||
adr x3, comps_cnt | ||
ldr x4, [x3] | ||
jacobbramley marked this conversation as resolved.
Show resolved
Hide resolved
|
||
add x4, x4, #1 | ||
str x4, [x3] | ||
|
||
// Update switcher DDC | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason this is commented? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented in 61b9d8a. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks good to me -- but can we also delete the commented out code if it's not needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 7e16316 for now. I guess we can keep the idea in mind, if we do want to take that approach. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lots of new code seems to have crept in with 7e16316 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 3defc97. (that was painful) |
||
//adr x2, switcher_caps | ||
//ldr c0, [x2] | ||
//gclen x1, c0 | ||
//add x1, x1, #32 | ||
//scbndse c0, c0, x1 | ||
//str c0, [x2] | ||
|
||
ret | ||
|
||
/** | ||
* Function to delete an existing compartment data | ||
* | ||
* @param c0 ID of compartment to be deleted | ||
*/ | ||
.type del_compartment, "function" | ||
del_compartment: | ||
jacobbramley marked this conversation as resolved.
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that for Vim swap files? Isn't it better to ignore those in a user
~/.gitignore
(or similar)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was, but it's pretty clearly bikeshedding so please disregard :-)