-
Notifications
You must be signed in to change notification settings - Fork 89
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
Add shared memory checks #743
base: oe_port
Are you sure you want to change the base?
Conversation
Also fixes #732. |
src/enclave/enclave_oe.c
Outdated
enc->virtio_net_dev_mem = host->virtio_net_dev_mem; | ||
CHECK_OUTSIDE(enc->virtio_net_dev_mem, sizeof(struct virtio_dev)); |
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.
virtio_dev
structure conains many pointers to sub structures, which also contain pointers to sub structures. Are those pointers suppoed to point to buffers outside of enclave? How to check and how to make sure a malicious host can't modify the pointers after the check?
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.
Even a friendly host will necessarily have to change the contents - that's what the shared memory is for. We will have to add checks on every memory access to any of those pointers in the lkl submodule.
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.
We should check the LKL virtio drivers do this. This is the same use as CVMs, so if they don't then we have a fairly simple attack via VirtIO on Linux on SEV and similar technologies (e.g. Google's recent offering). We may need to add something at the bus layer for hooks that the drivers use to check that memory is the correct kind. All of the descriptors that we use should be SWIOTLB-owned memory.
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.
Relevant dicussion at #745
f7d13f9
to
809808a
Compare
src/enclave/enclave_oe.c
Outdated
enc->virtio_net_dev_mem = host->virtio_net_dev_mem; | ||
CHECK_OUTSIDE(enc->virtio_net_dev_mem, sizeof(struct virtio_dev)); |
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.
We should check the LKL virtio drivers do this. This is the same use as CVMs, so if they don't then we have a fairly simple attack via VirtIO on Linux on SEV and similar technologies (e.g. Google's recent offering). We may need to add something at the bus layer for hooks that the drivers use to check that memory is the correct kind. All of the descriptors that we use should be SWIOTLB-owned memory.
809808a
to
223b8dc
Compare
6f40e1e
to
6ae4dd4
Compare
|
||
sgxlkl_shared_memory_t* enc = &sgxlkl_enclave_state.shared_memory; | ||
memset(enc, 0, sizeof(sgxlkl_shared_memory_t)); | ||
/* Temporary, volatile copy to make sure checks aren't reordered */ |
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.
host
points to a sgxlkl_shared_memory_t
structure copied into the enclave memory by OE. It's safe to access the structure inside the enclave directly, without worring about TOCTOU. Once all a check passes, the field from the sgxlkl_shared_memory_t
structure pointed by host
can be copied to sgxlkl_enclave_state.shared_memory directly, without using the volatile copy. If a shadow copy of a buffer pointed to by a pointer inside the sgxlkl_shared_memory_t
structure is used, the correspnding pointer in sgxlkl_enclave_state.shared_memory can be handled differently.
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.
OE only copies the outermost layer, it doesn't deep-copy. If something goes wrong here, we may end up with a partially initialized data structure, which may or may not be a security problem. I prefer to be conservative here and keep the copy. Since this runs once, it's also not a performance concern.
@@ -18,7 +18,6 @@ typedef struct enc_dev_config | |||
{ | |||
uint8_t dev_id; | |||
enc_evt_channel_t* enc_evt_chn; |
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.
Pointer chain here is unnecessarily complex. uint64_t enclave_evt_channel, uint64_t host_evt_channel, uint32_t qidx all shoud be in the share memory. Defining enc_dec_config as below should simplify the memory check in enclave.
{
uint8_t dev_id;
evt_t *enc_evt_channel;
evt_t *host_evt_channel;
uint32_t *qidx_p
}
```
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.
This brings a lot of complications in other parts of the code (e.g. during setup on the host side and host_evt_channel
and enclave_evt_channel
would have to be a evt_t**
) and I'm not sure those parts are even correct. @prp @davidchisnall: is it intended and correct that host_dev_event_channel_init
(here) allocates evt_channel_num
many enc_evt_channel_t
s for each event channel with number evt_channel_num
? Are these essentially (n^2)/2 communication links?
Signed-off-by: Christoph M. Wintersteiger <[email protected]>
Signed-off-by: Christoph M. Wintersteiger <[email protected]>
Signed-off-by: Christoph M. Wintersteiger <[email protected]>
Signed-off-by: Christoph M. Wintersteiger <[email protected]>
Signed-off-by: Christoph M. Wintersteiger <[email protected]>
Signed-off-by: Christoph M. Wintersteiger <[email protected]>
Signed-off-by: Christoph M. Wintersteiger <[email protected]>
Signed-off-by: Christoph M. Wintersteiger <[email protected]>
Signed-off-by: Christoph M. Wintersteiger <[email protected]>
Signed-off-by: Christoph M. Wintersteiger <[email protected]>
Signed-off-by: Christoph M. Wintersteiger <[email protected]>
Signed-off-by: Christoph M. Wintersteiger <[email protected]>
Signed-off-by: Christoph M. Wintersteiger <[email protected]>
6ae4dd4
to
1e58739
Compare
Signed-off-by: Christoph M. Wintersteiger <[email protected]>
Signed-off-by: Christoph M. Wintersteiger <[email protected]>
I think this one's ready to be merged. @SeanTAllen is working on the virtio refactoring, but that will be a separate PR. |
This adds a number of checks that ensure pointers into shared memory from the host don't point into enclave memory.
Also adds a check to ensure the enclave is initialized only once.