Skip to content

Commit

Permalink
Fix memory allocation in the runtime (#320)
Browse files Browse the repository at this point in the history
This change bundles two bugfixes to Eyrie's memory allocation system. The
first is a fix to the `brk` syscall which is used to allocate memory.
Often, calls to `brk` will use page aligned addresses as arguments --
but not always. The existing functionality in `brk` specifically runs
into problems in the `brk` call *after* such a non-page-aligned call.
For example, if we initially have `current_break = 0x2040000000`:

`brk(0x2040000B48)`: `req_page_count = 1`. In this case, we allocate one
page at VPN `0x2040000`.
`brk(0x0000002040021B48)`: `req_page_count = 33`. Since we earlier set
`current_break = 0x2040000B48`, we will accidentally *reallocate* VPN
`0x2040000` and thus not allocate one page at the end of this requested
break. This will result in segfaults on address `0x2040021388`, for
example.

The second fix to the `alloc_page` function, which now zeroes pages that
it allocates. I observed issues with some libcs (musl, specifically)
which assume (in the heap implementation) that pages returned from the
runtime are zeroed. Since this wasn't the case, some really cursed
memory corruptions were observed. This does come at a performance
overhead, but I believe that zeroing allocated pages is a pretty
standard choice in OS's.
  • Loading branch information
grg-haas committed Mar 24, 2023
1 parent 9089593 commit b25a682
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 4 deletions.
12 changes: 9 additions & 3 deletions runtime/call/linux_wrap.c
Original file line number Diff line number Diff line change
Expand Up @@ -170,11 +170,17 @@ uintptr_t syscall_brk(void* addr){
uintptr_t req_break = (uintptr_t)addr;

uintptr_t current_break = get_program_break();
uintptr_t ret = current_break;
uintptr_t ret;
int req_page_count = 0;

// Return current break if null or current break
if( req_break == 0 || req_break <= current_break){
if (req_break == 0) {
ret = current_break;
goto done;
}

if(req_break <= current_break){
ret = req_break;
goto done;
}

Expand All @@ -196,7 +202,7 @@ uintptr_t syscall_brk(void* addr){
}

// Success
set_program_break(req_break);
set_program_break(PAGE_UP(req_break));
ret = req_break;


Expand Down
2 changes: 1 addition & 1 deletion runtime/mm/mm.c
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ alloc_page(uintptr_t vpn, int flags)
}

/* otherwise, allocate one from the freemem */
page = spa_get();
page = spa_get_zero();
assert(page);

*pte = pte_create(ppn(__pa(page)), flags | PTE_V);
Expand Down

4 comments on commit b25a682

@acaldaya
Copy link
Contributor

Choose a reason for hiding this comment

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

@grg-haas who is expected to call this syscall, the runtime or the enclave?
IIUC, enclave heap is statically allocated in the linker script and handled by tiny_malloc.c

@grg-haas
Copy link
Collaborator Author

@grg-haas grg-haas commented on b25a682 Mar 25, 2023

Choose a reason for hiding this comment

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

@acaldaya Great question. These syscalls are primarily called by the eapp when it is using one of the libc implementations we support. Specifically, I've seen glibc rely more on brk while musl relies more on mmap. Both of these eventually call down to the generic alloc_page.

@acaldaya
Copy link
Contributor

@acaldaya acaldaya commented on b25a682 Mar 25, 2023

Choose a reason for hiding this comment

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

interesting
is FreeMem support sufficiently complete to rely only on the RT dynamic memory allocator? (i.e. not using tiny-malloc at all)
my understanding is that it wasn't due to some comments in SDK code.

@grg-haas
Copy link
Collaborator Author

@grg-haas grg-haas commented on b25a682 Mar 25, 2023

Choose a reason for hiding this comment

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

I have been using freemem almost exclusively since I've started working on Keystone (half a year), and haven't run into any issues personally! I'm not quite sure what that comment is about, but may be worth revisiting the SDK in the future

Please sign in to comment.