Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

Commit

Permalink
Remove partially-implemented static guard pages
Browse files Browse the repository at this point in the history
It turns out that our guard pages were incorrectly handled (i.e. not re-
added to LibOS VMA list) on SGX when execve was optimized to re-use the
same enclave, which caused exec_same test to crash from time to time
(when ASLR put heap on a guard page).

Static guard pages aren't too useful and introduce unnecessary
complexity to our code, so we decided to just delete them in order to
fix this bug.
  • Loading branch information
mkow committed Jul 21, 2020
1 parent 1895f34 commit b789ed3
Show file tree
Hide file tree
Showing 12 changed files with 27 additions and 54 deletions.
19 changes: 0 additions & 19 deletions LibOS/shim/src/bookkeep/shim_vma.c
Original file line number Diff line number Diff line change
Expand Up @@ -507,25 +507,6 @@ int init_vma(void) {
.offset = 0,
.comment = "exec",
},
/* TODO: remove these 2 guard pages, they should not exist (but Linux-SGX Pal adds them). */
{
.begin = (uintptr_t)PAL_CB(executable_range.start) - PAL_CB(exec_memory_gap),
.end = (uintptr_t)PAL_CB(executable_range.start),
.prot = PROT_NONE,
.flags = MAP_PRIVATE | MAP_ANONYMOUS | VMA_UNMAPPED,
.file = NULL,
.offset = 0,
.comment = "guard_page",
},
{
.begin = (uintptr_t)PAL_CB(executable_range.end),
.end = (uintptr_t)PAL_CB(executable_range.end) + PAL_CB(exec_memory_gap),
.prot = PROT_NONE,
.flags = MAP_PRIVATE | MAP_ANONYMOUS | VMA_UNMAPPED,
.file = NULL,
.offset = 0,
.comment = "guard_page",
},
};

spinlock_lock_signal_off(&vma_tree_lock);
Expand Down
1 change: 0 additions & 1 deletion Pal/include/pal/pal.h
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,6 @@ typedef struct PAL_CONTROL_ {
PAL_PTR_RANGE user_address; /*!< The range of user addresses */

PAL_PTR_RANGE executable_range; /*!< address where executable is loaded */
PAL_NUM exec_memory_gap; /*!< Size of memory gap before and after executable */
PAL_PTR_RANGE manifest_preload; /*!< manifest preloaded here */

/*
Expand Down
3 changes: 1 addition & 2 deletions Pal/src/db_main.c
Original file line number Diff line number Diff line change
Expand Up @@ -504,8 +504,7 @@ noreturn void pal_main(
g_pal_control.disable_aslr = disable_aslr;

_DkGetAvailableUserAddressRange(&g_pal_control.user_address.start,
&g_pal_control.user_address.end,
&g_pal_control.exec_memory_gap);
&g_pal_control.user_address.end);

g_pal_control.alloc_align = g_pal_state.alloc_align;

Expand Down
11 changes: 5 additions & 6 deletions Pal/src/host/Linux-SGX/db_main.c
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ unsigned long _DkGetAllocationAlignment (void)
return g_page_size;
}

void _DkGetAvailableUserAddressRange(PAL_PTR* start, PAL_PTR* end, PAL_NUM* gap) {
void _DkGetAvailableUserAddressRange(PAL_PTR* start, PAL_PTR* end) {
*start = (PAL_PTR)g_pal_sec.heap_min;
*end = (PAL_PTR)get_enclave_heap_top();

Expand All @@ -55,8 +55,6 @@ void _DkGetAvailableUserAddressRange(PAL_PTR* start, PAL_PTR* end, PAL_NUM* gap)
SGX_DBG(DBG_E, "Not enough enclave memory, please increase enclave size!\n");
ocall_exit(1, /*is_exitgroup=*/true);
}

*gap = MEMORY_GAP;
}

PAL_NUM _DkGetProcessId (void)
Expand Down Expand Up @@ -220,9 +218,10 @@ void pal_linux_main(char* uptr_args, uint64_t args_size, char* uptr_env, uint64_
void* zero2_end = g_pal_sec.heap_max;

if (g_pal_sec.exec_addr != NULL) {
zero1_end = MIN(zero1_end, SATURATED_P_SUB(g_pal_sec.exec_addr, MEMORY_GAP, 0));
zero2_start = SATURATED_P_ADD(g_pal_sec.exec_addr + g_pal_sec.exec_size, MEMORY_GAP,
zero2_end);
zero1_end = g_pal_sec.exec_addr;
zero2_start = g_pal_sec.exec_addr + g_pal_sec.exec_size;
assert(zero1_start <= zero1_end);
assert(zero2_start <= zero2_end);
}

memset(zero1_start, 0, zero1_end - zero1_start);
Expand Down
5 changes: 2 additions & 3 deletions Pal/src/host/Linux-SGX/enclave_pages.c
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,8 @@ int init_enclave_pages(void) {
goto out;
}

exec_vma->bottom = SATURATED_P_SUB(g_pal_sec.exec_addr, MEMORY_GAP, g_heap_bottom);
exec_vma->top = SATURATED_P_ADD(g_pal_sec.exec_addr + g_pal_sec.exec_size, MEMORY_GAP,
g_heap_top);
exec_vma->bottom = g_pal_sec.exec_addr;
exec_vma->top = g_pal_sec.exec_addr + g_pal_sec.exec_size;
exec_vma->is_pal_internal = false;
INIT_LIST_HEAD(exec_vma, list);
LISTP_ADD(exec_vma, &g_heap_vma_list, list);
Expand Down
1 change: 0 additions & 1 deletion Pal/src/host/Linux-SGX/generated-offsets.c
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,6 @@ void dummy(void)
/* pal_linux_def.h */
DEFINE(ENCLAVE_HIGH_ADDRESS, ENCLAVE_HIGH_ADDRESS);
DEFINE(SSAFRAMENUM, SSAFRAMENUM);
DEFINE(MEMORY_GAP, MEMORY_GAP);
DEFINE(ENCLAVE_STACK_SIZE, ENCLAVE_STACK_SIZE);
DEFINE(ENCLAVE_SIG_STACK_SIZE, ENCLAVE_SIG_STACK_SIZE);
DEFINE(DEFAULT_HEAP_MIN, DEFAULT_HEAP_MIN);
Expand Down
1 change: 0 additions & 1 deletion Pal/src/host/Linux-SGX/pal_linux_defs.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

#define ENCLAVE_HIGH_ADDRESS 0x800000000
#define SSAFRAMENUM 2
#define MEMORY_GAP PRESET_PAGESIZE
#define ENCLAVE_STACK_SIZE (PRESET_PAGESIZE * 64)
#define ENCLAVE_SIG_STACK_SIZE (PRESET_PAGESIZE * 16)
#define DEFAULT_HEAP_MIN 0x10000
Expand Down
16 changes: 8 additions & 8 deletions Pal/src/host/Linux-SGX/sgx_main.c
Original file line number Diff line number Diff line change
Expand Up @@ -464,25 +464,25 @@ static int initialize_enclave(struct pal_enclave* enclave) {
}

unsigned long populating = enclave->size;
for (int i = 0 ; i < area_num ; i++) {
for (int i = 0; i < area_num; i++) {
if (areas[i].addr)
continue;
areas[i].addr = populating - areas[i].size;
populating = SATURATED_P_SUB(areas[i].addr, MEMORY_GAP, 0);
populating = areas[i].addr;
}

enclave_entry_addr += pal_area->addr;

if (exec_area) {
if (exec_area->addr + exec_area->size > pal_area->addr - MEMORY_GAP) {
if (exec_area->addr + exec_area->size > pal_area->addr) {
SGX_DBG(DBG_E, "Application binary overlaps with Pal binary\n");
ret = -EINVAL;
goto out;
}

if (exec_area->addr + exec_area->size + MEMORY_GAP < populating) {
if (exec_area->addr + exec_area->size < populating) {
if (populating > heap_min) {
unsigned long addr = exec_area->addr + exec_area->size + MEMORY_GAP;
unsigned long addr = exec_area->addr + exec_area->size;
if (addr < heap_min)
addr = heap_min;

Expand All @@ -494,7 +494,7 @@ static int initialize_enclave(struct pal_enclave* enclave) {
area_num++;
}

populating = SATURATED_P_SUB(exec_area->addr, MEMORY_GAP, 0);
populating = exec_area->addr;
}
}

Expand Down Expand Up @@ -550,8 +550,8 @@ static int initialize_enclave(struct pal_enclave* enclave) {
gs->gpr = gs->ssa +
enclave->ssaframesize - sizeof(sgx_pal_gpr_t);
gs->manifest_size = manifest_size;
gs->heap_min = (void *) enclave_secs.base + heap_min;
gs->heap_max = (void *) enclave_secs.base + pal_area->addr - MEMORY_GAP;
gs->heap_min = (void*)enclave_secs.base + heap_min;
gs->heap_max = (void*)enclave_secs.base + pal_area->addr;
if (exec_area) {
gs->exec_addr = (void *) enclave_secs.base + exec_area->addr;
gs->exec_size = exec_area->size;
Expand Down
10 changes: 5 additions & 5 deletions Pal/src/host/Linux-SGX/signer/pal_sgx_sign.py
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,7 @@ def set_tcs_field(t, offset, pack_fmt, value):
def set_tls_field(t, offset, value):
struct.pack_into('<Q', tls_data, t * offs.PAGESIZE + offset, value)

enclave_heap_max = pal_area.addr - offs.MEMORY_GAP
enclave_heap_max = pal_area.addr

# Sanity check that we measure everything except the heap which is zeroed
# on enclave startup.
Expand Down Expand Up @@ -509,17 +509,17 @@ def populate_memory_areas(attr, areas):
area.addr = populating - area.size
if area.addr < ENCLAVE_HEAP_MIN:
raise Exception("Enclave size is not large enough")
populating = max(area.addr - offs.MEMORY_GAP, 0)
populating = area.addr

free_areas = []
for area in areas:
if area.addr + area.size + offs.MEMORY_GAP < populating:
addr = area.addr + area.size + offs.MEMORY_GAP
addr = area.addr + area.size
if addr < populating:
flags = PAGEINFO_R | PAGEINFO_W | PAGEINFO_X | PAGEINFO_REG
free_areas.append(
MemoryArea('free', addr=addr, size=populating - addr,
flags=flags, measure=False))
populating = max(area.addr - offs.MEMORY_GAP, 0)
populating = area.addr

if populating > ENCLAVE_HEAP_MIN:
flags = PAGEINFO_R | PAGEINFO_W | PAGEINFO_X | PAGEINFO_REG
Expand Down
10 changes: 4 additions & 6 deletions Pal/src/host/Linux/db_main.c
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ unsigned long _DkGetAllocationAlignment (void)
return g_page_size;
}

void _DkGetAvailableUserAddressRange(PAL_PTR* start, PAL_PTR* end, PAL_NUM* gap) {
void _DkGetAvailableUserAddressRange(PAL_PTR* start, PAL_PTR* end) {
void* end_addr = (void*)ALLOC_ALIGN_DOWN_PTR(TEXT_START);
void* start_addr = (void*)USER_ADDRESS_LOWEST;

Expand All @@ -127,13 +127,11 @@ void _DkGetAvailableUserAddressRange(PAL_PTR* start, PAL_PTR* end, PAL_NUM* gap)
break;
}

start_addr = (void *) ((unsigned long) start_addr << 1);
start_addr = (void*)((unsigned long)start_addr << 1);
}

*end = (PAL_PTR) end_addr;
*start = (PAL_PTR) start_addr;

*gap = 0;
*end = (PAL_PTR)end_addr;
*start = (PAL_PTR)start_addr;
}

PAL_NUM _DkGetProcessId(void) {
Expand Down
2 changes: 1 addition & 1 deletion Pal/src/host/Skeleton/db_main.c
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ unsigned long _DkGetAllocationAlignment (void)
return 0;
}

void _DkGetAvailableUserAddressRange(PAL_PTR* start, PAL_PTR* end, PAL_NUM* gap) {
void _DkGetAvailableUserAddressRange(PAL_PTR* start, PAL_PTR* end) {
/* needs to be implemented */
}

Expand Down
2 changes: 1 addition & 1 deletion Pal/src/pal_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ noreturn void pal_main(

/* For initialization */
unsigned long _DkGetAllocationAlignment (void);
void _DkGetAvailableUserAddressRange(PAL_PTR* start, PAL_PTR* end, PAL_NUM* gap);
void _DkGetAvailableUserAddressRange(PAL_PTR* start, PAL_PTR* end);
bool _DkCheckMemoryMappable (const void * addr, size_t size);
PAL_NUM _DkGetProcessId (void);
PAL_NUM _DkGetHostId (void);
Expand Down

0 comments on commit b789ed3

Please sign in to comment.