Skip to content
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

Open
wants to merge 15 commits into
base: oe_port
Choose a base branch
from

Conversation

wintersteiger
Copy link
Contributor

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.

@wintersteiger
Copy link
Contributor Author

Also fixes #732.

src/lkl/virtio_netdev.c Outdated Show resolved Hide resolved
enc->virtio_net_dev_mem = host->virtio_net_dev_mem;
CHECK_OUTSIDE(enc->virtio_net_dev_mem, sizeof(struct virtio_dev));
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relevant dicussion at #745

src/enclave/enclave_oe.c Outdated Show resolved Hide resolved
src/enclave/enclave_event_channel.c Outdated Show resolved Hide resolved
src/enclave/enclave_event_channel.c Outdated Show resolved Hide resolved
src/enclave/enclave_oe.c Outdated Show resolved Hide resolved
enc->virtio_net_dev_mem = host->virtio_net_dev_mem;
CHECK_OUTSIDE(enc->virtio_net_dev_mem, sizeof(struct virtio_dev));
Copy link
Contributor

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.

src/enclave/enclave_timer.c Outdated Show resolved Hide resolved
src/enclave/enclave_oe.c Outdated Show resolved Hide resolved
src/lkl/setup.c Outdated Show resolved Hide resolved

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 */
Copy link
Contributor

@bodzhang bodzhang Sep 1, 2020

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.

Copy link
Contributor Author

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.

src/enclave/enclave_oe.c Outdated Show resolved Hide resolved
src/enclave/enclave_oe.c Outdated Show resolved Hide resolved
@@ -18,7 +18,6 @@ typedef struct enc_dev_config
{
uint8_t dev_id;
enc_evt_channel_t* enc_evt_chn;
Copy link
Contributor

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
}
```

Copy link
Contributor Author

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_ts 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]>
Signed-off-by: Christoph M. Wintersteiger <[email protected]>
@wintersteiger
Copy link
Contributor Author

I think this one's ready to be merged. @SeanTAllen is working on the virtio refactoring, but that will be a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants