Skip to content

Commit

Permalink
Switched some of the SBI calls over to a struct-passing system. Only …
Browse files Browse the repository at this point in the history
…CREATE and RUN have changed for now.
  • Loading branch information
David Kohlbrenner committed Nov 1, 2018
1 parent c9718ce commit 6ed5bd9
Show file tree
Hide file tree
Showing 6 changed files with 121 additions and 20 deletions.
11 changes: 6 additions & 5 deletions machine/mtrap.c
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ void mcall_trap(uintptr_t* regs, uintptr_t mcause, uintptr_t mepc)
{
write_csr(mepc, mepc + 4);

uintptr_t n = regs[17], arg0 = regs[10], arg1 = regs[11], arg2 = regs[12], arg3 = regs[13], retval, ipi_type;
uintptr_t n = regs[17], arg0 = regs[10], arg1 = regs[11], arg2 = regs[12], arg3 = regs[13], retval, ipi_type, entry_point;

switch (n)
{
Expand Down Expand Up @@ -166,17 +166,18 @@ void mcall_trap(uintptr_t* regs, uintptr_t mcause, uintptr_t mepc)
break;
#ifdef SM_ENABLED
case SBI_SM_CREATE_ENCLAVE:
retval = mcall_sm_create_enclave(arg0, arg1, arg2);
retval = mcall_sm_create_enclave(arg0);
break;
case SBI_SM_DESTROY_ENCLAVE:
retval = mcall_sm_destroy_enclave(arg0);
break;
case SBI_SM_RUN_ENCLAVE:
retval = mcall_sm_run_enclave(regs, arg0, arg1, arg2);
retval = mcall_sm_run_enclave(regs, arg0, &entry_point);
if(!retval)
{
/* the entry point is passed to the runtime through $a0 */
retval = arg1;
/* TODO: have a cleaner way to do this
the entry point is passed to the runtime through $a0 */
retval = entry_point;
}
break;
case SBI_SM_EXIT_ENCLAVE:
Expand Down
49 changes: 44 additions & 5 deletions sm/enclave.c
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ int detect_region_overlap(unsigned int eid, uintptr_t addr, uintptr_t size)
((uintptr_t) epm_base + epm_size > addr);
}

void copy_word_to_host(uintptr_t* ptr, uintptr_t value)
int copy_word_to_host(uintptr_t* ptr, uintptr_t value)
{
int region_overlap = 0, i;
spinlock_lock(&encl_lock);
Expand All @@ -109,9 +109,37 @@ void copy_word_to_host(uintptr_t* ptr, uintptr_t value)
}
if(!region_overlap)
*ptr = value;
spinlock_unlock(&encl_lock);

if(region_overlap)
return ENCLAVE_REGION_OVERLAPS;
else
*ptr = -1UL;
return ENCLAVE_SUCCESS;
}


// Does not do checking of dest!
int copy_region_from_host(void* source, void* dest, size_t size){

This comment has been minimized.

Copy link
@dayeol

dayeol Nov 4, 2018

Contributor

@dkohlbre See comments below,
Current usecase of this function only copies the OS input to SM local memory, which does not have to be checked for the overlaps with enclaves.
This function could be useful when the untrusted OS copies its data into an arbitrary physical address that the OS provides.
However, I think that the better way is to treat "untrusted memory" as an additional PMP region, and move overlap detection to region creation.
Then the PMP region can be referred by both of the OS (host) and the enclave using their own mapped virtual address, not physical address.


int region_overlap = 0, i;
spinlock_lock(&encl_lock);
for(i=0; i<ENCL_MAX; i++)
{
if(!TEST_BIT(encl_bitmap, i))
continue;
region_overlap |= detect_region_overlap(i, (uintptr_t)source, size);
if(region_overlap)
break;
}
if(!region_overlap)
memcpy(dest, source, size);
spinlock_unlock(&encl_lock);

if(region_overlap)
return ENCLAVE_REGION_OVERLAPS;
else
return ENCLAVE_SUCCESS;

}

int init_enclave_memory(uintptr_t base, uintptr_t size)
Expand All @@ -129,9 +157,13 @@ int init_enclave_memory(uintptr_t base, uintptr_t size)

return ret;
}

enclave_ret_t create_enclave(uintptr_t base, uintptr_t size, unsigned int* eidptr)
enclave_ret_t create_enclave(struct keystone_sbi_create_t create_args)
//enclave_ret_t create_enclave(uintptr_t base, uintptr_t size, unsigned int* eidptr)
{
uintptr_t base = create_args.epm_region.paddr;
size_t size = create_args.epm_region.size;
unsigned int* eidptr = create_args.eid_pptr;

uint8_t perm = 0;
unsigned int eid;
enclave_ret_t ret;
Expand Down Expand Up @@ -241,8 +273,12 @@ enclave_ret_t destroy_enclave(unsigned int eid)

#define RUNTIME_START_ADDRESS 0xffffffffc0000000

enclave_ret_t run_enclave(uintptr_t* host_regs, unsigned int eid, uintptr_t entry, unsigned long* retptr)
enclave_ret_t run_enclave(uintptr_t* host_regs, struct keystone_sbi_run_t run_args)
{
unsigned int eid = run_args.eid;
uintptr_t entry = run_args.entry_ptr;
unsigned long* retptr = run_args.ret_ptr;

int runable;

spinlock_lock(&encl_lock);
Expand All @@ -269,6 +305,9 @@ enclave_ret_t run_enclave(uintptr_t* host_regs, unsigned int eid, uintptr_t entr

/* save host context */
swap_prev_state(&enclaves[eid].threads[0], host_regs);

/* Swapping the mepc sets up the mret in mtrap.c to transfer control
to the enclave */
swap_prev_mepc(&enclaves[eid].threads[0], read_csr(mepc));
swap_prev_stvec(&enclaves[eid].threads[0], read_csr(stvec));

Expand Down
8 changes: 6 additions & 2 deletions sm/enclave.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

#include "pmp.h"
#include "thread.h"
#include "keystone-sbi-arg.h"
#include "sm.h"

/* TODO: does not support multithreaded enclave yet */
#define MAX_ENCL_THREADS 1
Expand Down Expand Up @@ -32,11 +34,13 @@ struct enclave_t
struct thread_state_t threads[MAX_ENCL_THREADS];
};

int copy_region_from_host(void* source, void* dest, size_t size);

unsigned long get_host_satp(unsigned int eid);
enclave_ret_t create_enclave(uintptr_t base, size_t size, unsigned int* eidptr);
enclave_ret_t create_enclave(struct keystone_sbi_create_t create_args);
enclave_ret_t destroy_enclave(unsigned int eid);

enclave_ret_t run_enclave(uintptr_t* regs, unsigned int eid, uintptr_t entry, unsigned long* retptr);
enclave_ret_t run_enclave(uintptr_t* host_regs, struct keystone_sbi_run_t run_args);
enclave_ret_t exit_enclave(uintptr_t* regs, unsigned long retval);
enclave_ret_t stop_enclave(uintptr_t* regs, uint64_t request);
enclave_ret_t resume_enclave(uintptr_t* regs, unsigned int eid);
Expand Down
34 changes: 34 additions & 0 deletions sm/keystone-sbi-arg.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
#ifndef _KEYSTONE_SBI_ARG_H_
#define _KEYSTONE_SBI_ARG_H_

struct keystone_sbi_pregion_t
{
uintptr_t paddr;
size_t size;
};

struct keystone_sbi_create_t
{
// Memory regions for the enclave
struct keystone_sbi_pregion_t epm_region;
struct keystone_sbi_pregion_t copy_region;

// Outputs from the creation process
unsigned int* eid_pptr;
};


struct keystone_sbi_run_t
{
unsigned int eid;
uintptr_t entry_ptr;
uintptr_t* ret_ptr;
};

struct keystone_sbi_general_t
{
unsigned int eid;
};


#endif
33 changes: 27 additions & 6 deletions sm/sm-sbi.c
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,19 @@
#include "enclave.h"
#include <errno.h>

uintptr_t mcall_sm_create_enclave(unsigned long base, unsigned long size, unsigned long eidptr)

uintptr_t mcall_sm_create_enclave(uintptr_t create_args)
{
struct keystone_sbi_create_t create_args_local;
enclave_ret_t ret;
ret = create_enclave((uintptr_t) base, (size_t) size, (unsigned int*) eidptr);
ret = copy_region_from_host((struct keystone_sbi_create_t*)create_args,

This comment has been minimized.

Copy link
@dayeol

dayeol Nov 4, 2018

Contributor

@dkohlbre Why do we have to pass create_enclave arguments like this?
I think there's no need for an extra copy

&create_args_local,
sizeof(struct keystone_sbi_create_t));

if( ret != ENCLAVE_SUCCESS )
return ret;

ret = create_enclave(create_args_local);
return ret;
}

Expand All @@ -18,12 +27,24 @@ uintptr_t mcall_sm_destroy_enclave(unsigned long eid)
ret = destroy_enclave((unsigned int)eid);
return ret;
}

uintptr_t mcall_sm_run_enclave(uintptr_t* host_regs, unsigned long eid, unsigned long ptr, unsigned long retval)
uintptr_t mcall_sm_run_enclave(uintptr_t* regs, uintptr_t run_args, uintptr_t* entry_point)
{
if(get_host_satp(eid) != read_csr(satp))
struct keystone_sbi_run_t run_args_local;
enclave_ret_t ret;
ret = copy_region_from_host((struct keystone_sbi_run_t*)run_args,

This comment has been minimized.

Copy link
@dayeol

dayeol Nov 4, 2018

Contributor

@dkohlbre I think we don't need extra copy here as well.

&run_args_local,
sizeof(struct keystone_sbi_run_t));

if( ret != ENCLAVE_SUCCESS )
return ret;

if(get_host_satp(run_args_local.eid) != read_csr(satp))
return ENCLAVE_NOT_ACCESSIBLE;
return run_enclave(host_regs, (unsigned int) eid, (uintptr_t) ptr, (unsigned long*) retval);

ret = run_enclave(regs, run_args_local);
if( ret == ENCLAVE_SUCCESS )
*entry_point = run_args_local.entry_ptr;
return ret;
}

uintptr_t mcall_sm_resume_enclave(uintptr_t* host_regs, unsigned long eid)
Expand Down
6 changes: 4 additions & 2 deletions sm/sm-sbi.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@

#include <stdint.h>
#include <stddef.h>
#include "keystone-sbi-arg.h"

typedef uintptr_t enclave_ret_t;

uintptr_t mcall_sm_create_enclave(unsigned long base, unsigned long size, unsigned long eidptr);
uintptr_t mcall_sm_create_enclave(uintptr_t create_args);

uintptr_t mcall_sm_destroy_enclave(unsigned long eid);

uintptr_t mcall_sm_run_enclave(uintptr_t* regs, unsigned long eid, unsigned long ptr, unsigned long retptr);
uintptr_t mcall_sm_run_enclave(uintptr_t* regs, uintptr_t run_args, uintptr_t* entry_point);
uintptr_t mcall_sm_exit_enclave(uintptr_t* regs, unsigned long retval);
uintptr_t mcall_sm_not_implemented(uintptr_t* regs, unsigned long a0);
uintptr_t mcall_sm_stop_enclave(uintptr_t* regs, unsigned long request);
Expand Down

0 comments on commit 6ed5bd9

Please sign in to comment.