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

OpenEnclave PAL: Store enclave heap base/end in inline variables. #201

Merged
merged 1 commit into from
May 28, 2020
Merged

OpenEnclave PAL: Store enclave heap base/end in inline variables. #201

merged 1 commit into from
May 28, 2020

Conversation

anakrish
Copy link
Contributor

@anakrish anakrish commented May 25, 2020

PALOpenEnclave object is lazily constructed. I couldn't
figure out a straight-forward way to pass the heap bounds to
the constructor of PALOpenEnclave object.
As an alternative, store the bounds in class-static members of a template
class.

  • two_alloc_types/main.cc
    OE use of snmalloc is renamed to snmalloc_enclave.
    Declare oe_base and oe_end inline variables in snmalloc_enclave
    namespace and initialize them.

  • fixed_region/fixed_region.cc
    Initialize oe_base and oe_end variables.
    Cache the starting value of oe_base in orig_oe_base and
    use that for asserting that the allocated memory lies
    within the PAL heap. This is because, in pal_open_enclave.h,
    reserve will change the value of oe_base.

CC: @achamayou
Signed-off-by: Anand Krishnamoorthi [email protected]

Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

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

You need to update the tests too:

The following code can be dropped:

void* oe_base;
void* oe_end;
extern "C" const void* __oe_get_heap_base()
{
return oe_base;
}
extern "C" const void* __oe_get_heap_end()
{
return oe_end;
}

And then you need to change

oe_base = mp.reserve<true>(large_class);
oe_end = (uint8_t*)oe_base + size;

to update the inline statics.

Similar in here

oe_base = mp.reserve<true>(large_class);
oe_end = (uint8_t*)oe_base + size;

src/pal/pal_open_enclave.h Outdated Show resolved Hide resolved
src/pal/pal_open_enclave.h Outdated Show resolved Hide resolved
@anakrish anakrish changed the title OpenEnclave PAL: Store enclave heap base/end in class-statics. OpenEnclave PAL: Store enclave heap base/end in inline variables. May 27, 2020
@anakrish
Copy link
Contributor Author

@mjp41 Thanks for the suggestions. Can you review the latest iteration?

Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

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

@anakrish thanks for the contribution. I am happy with this as is. I think making the static method on PalOpenEnclave would be nicer. But I am no bothered enough, so if you have time, change it. But otherwise, we can leave as is.

src/pal/pal_open_enclave.h Outdated Show resolved Hide resolved
Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

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

Thanks for updating it. Just a minor clangformat issue, then this is good to go.

You can do

ninja clangformat

to run clangformat with the relevant settings.

src/test/func/two_alloc_types/main.cc Outdated Show resolved Hide resolved

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
PALOpenEnclave object is lazily constructed. I couldn't
figure out a straight-forward way to pass the heap bounds to
the constructor of PALOpenEnclave object.
As an alternative, store the bounds in inline static variables of
the PALOpenEnclave class and set them via static setup_initial_range
function.

- two_alloc_types/alloc1.cc
  Define oe_allocator_init to forward base, end values to
  PALOpenEnclave::setup_inital_range
- two_alloc_types/main.cc
  Use oe_allocator_init function to set up heap range.

- fixed_region/fixed_region.cc
  Initialize heap range via call to PALOpenEnclave::setup_inital_range.

Signed-off-by: Anand Krishnamoorthi <[email protected]>
@anakrish
Copy link
Contributor Author

Thanks for updating it. Just a minor clangformat issue, then this is good to go.

You can do

ninja clangformat

to run clangformat with the relevant settings.

Done. Looks like the CI is happy too.

Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

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

LGTM

@mjp41 mjp41 merged commit c7736a2 into microsoft:master May 28, 2020
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

2 participants