User-space separation #815

mikbras · 2020-08-21T15:36:43Z

This PR, separates the C runtime (libc and ldso) into its own ELF memory region, effectively isolating it from the kernel. The resulting library is called libsgxlkl-user.so (built under the ./user directory).

This PR depends on two other PRs:
- lsds/sgx-lkl-musl#37
- openenclave/openenclave#3425

For more information see the README below.

https://github.com/lsds/sgx-lkl/pull/815/files?short_path=9e7b9bd#diff-9e7b9bdf8e7469060b7afdf451e7e430

davidchisnall · 2020-08-24T08:46:36Z

src/enclave/enclave_init.c

+static long _lkl_syscall_wrapper(long no, long* params)
+{
+    //sgxlkl_warn("syscall begin: no=%u\n", no);
+    long ret = lkl_syscall(no, params);
+    //sgxlkl_warn("syscall end: ret=%u\n", ret);
+    return ret;
+}
+


This looks like dead code that was left in during debugging.

Suggested change

static long _lkl_syscall_wrapper(long no, long* params)

{

//sgxlkl_warn("syscall begin: no=%u\n", no);

long ret = lkl_syscall(no, params);

//sgxlkl_warn("syscall end: ret=%u\n", ret);

return ret;

}

davidchisnall · 2020-08-24T08:47:09Z

src/enclave/enclave_init.c

+    if (!(proc = __oe_get_isolated_image_entry_point()))
+        sgxlkl_fail("failed to obtain user space entry point");
+
+    args.ua_lkl_syscall = _lkl_syscall_wrapper;


See above comment about dead code.

Suggested change

args.ua_lkl_syscall = _lkl_syscall_wrapper;

args.ua_lkl_syscall = lkl_syscall;

davidchisnall · 2020-08-24T08:51:07Z

src/enclave/enclave_init.c

+    return ret;
+}
+
+static void _enter_user_space(


This is fine for now, but we should be setting up the stack in the kernel and using a clone syscall to launch the thread on a pre-prepared stack using the libc entry point.

davidchisnall · 2020-08-24T11:20:50Z

user/stubs.c

+long lkl_syscall(long no, long* params)
+{
+    long ret = __sgxlkl_userargs->ua_lkl_syscall(no, params);
+
+    return ret;
+}


Fine for now, but please open an issue to track the fact that we should be always referencing lkl_syscall via a function pointer (or, ideally, an ifunc).

davidchisnall · 2020-08-24T11:21:11Z

user/stubs.c

+**==============================================================================
+*/
+
+long lkl_syscall(long no, long* params)


Does this need a hidden-visibility attribute, or is the build system still hiding this from userspace?

davidchisnall · 2020-08-24T11:23:19Z

user/stubs.c

+    int params_len,
+    ...)
+{
+    sgxlkl_warn("__sgxlkl_log_syscall() unimplemented in user space");


This is still called for dup2 and epoll_wait (redirected system calls). I think we should just remove the caller here, since we're removing the behaviour.

davidchisnall · 2020-08-24T11:24:33Z

user/stubs.c

+void sgxlkl_warn(const char* msg, ...)
+{
+    /* ATTN: ignore variadic arguments */
+    return __sgxlkl_userargs->ua_sgxlkl_warn(msg);
+}
+
+void sgxlkl_error(const char* msg, ...)
+{
+    /* ATTN: ignore variadic arguments */
+    return __sgxlkl_userargs->ua_sgxlkl_error(msg);
+}
+
+void sgxlkl_fail(const char* msg, ...)
+{
+    /* ATTN: ignore variadic arguments */
+    return __sgxlkl_userargs->ua_sgxlkl_fail(msg);
+}


We shouldn't be calling any kernel code other than the syscall interface in userspace. Please can you conditionally compile these so that they appear only in debug builds?

davidchisnall · 2020-08-24T11:28:37Z

user/stubs.c

+    return __sgxlkl_userargs->ua_enclave_mmap(addr, length, mmap_fixed,
+        prot, zero_pages);


Userspace's libc shouldn't issue this call directly. We probably need to still call enclave_mmap because the kernel libc tries to mmap things before LKL is running, but this one should just do an mmap syscall.

Suggested change

return __sgxlkl_userargs->ua_enclave_mmap(addr, length, mmap_fixed,

prot, zero_pages);

return mmap(addr, length, prot, MAP_PRIVATE | MAP_ANONYMOUS | (mmap_fixed ? MAP_FIXED : 0, -1, 0);

This is called in two places in malloc, neither sets map_fixed, so we don't actually need to handle this case. It might be worth adding an assertion, something like:

Suggested change

return __sgxlkl_userargs->ua_enclave_mmap(addr, length, mmap_fixed,

prot, zero_pages);

assert(prot == PROT_READ | PROT_WRITE);

assert(!mmap_fixed);

return mmap(addr, length, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);

This can then be removed once we are no longer linking a copy of libc into the kernel.

davidchisnall · 2020-08-24T11:30:52Z

This PR contains some calls to functions that shouldn't exist. I've opened #816 to track this - I don't believe the fix should block this PR landing.

mikbras · 2020-08-24T15:19:40Z

Thank you David for your review. I'm sorry I created the PR without much background. Here are a few follow-up issues:

There are still some syscall bypasses that are beyond the scope of this PR to remove (see userargs.h).
This PR breaks syscall logging. We need to implement syslog as soon as possible.
A few builtin math functions are stubbed out (see stubs.c).

Also, the tests are broken since I forgot to add an install rule for libsgxlkl-user.so. I will update the PR in a couple of hours.

davidchisnall · 2020-08-24T15:29:57Z

We moved syscall logging into the kernel already, the only thing that's broken is locking for translated syscalls (ones where we implement a legacy x86 syscall by calling its legacy version). I don't think we actually need that.

letmaik · 2020-08-24T16:07:26Z

user/stubs.c

+#pragma GCC diagnostic ignored "-Wbuiltin-declaration-mismatch"
+
+void __muldc3()
+{


I would call abort() here, just in case they actually get called.

vtikoo · 2020-08-25T19:40:23Z

src/main-oe/sgxlkl_run_oe.c

+/* Determines path of libsgxlkl-user */
+void get_libsgxlkl_user_path(char* path_buf, size_t len)
+{
+    find_lib("libsgxlkl-user.so", path_buf, len);


For my understanding, is this shared object not signed like the kernel counterpart?

vtikoo · 2020-08-25T19:41:49Z

src/main-oe/sgxlkl_run_oe.c

@@ -1391,8 +1405,15 @@ void _create_enclave(

    setting.u.eeid = eeid;

+    // Format the follwing path <libsgxlkl>:<libsgxlkl_user>


Suggested change

// Format the follwing path <libsgxlkl>:<libsgxlkl_user>

// Format the following path <libsgxlkl>:<libsgxlkl_user>

vtikoo · 2020-08-25T20:21:08Z

src/main-oe/sgxlkl_run_oe.c

@@ -1863,6 +1891,13 @@ int main(int argc, char* argv[], char* envp[])
    }
    sgxlkl_host_verbose_raw("result=%s\n", libsgxlkl);

+    sgxlkl_host_verbose("get_libsgxlkl_user_path... ");
+    if (!isolated_image_provided)


This will always evaluate to true, I don't see where isolated_image_provided is being set again after initialization.

Maybe should happen on L1774?

vtikoo · 2020-08-25T22:22:54Z

src/enclave/enclave_init.c

    {
-        sgxlkl_fail("Failed to create lthread for startmain()\n");
+        static startmain_args_t args;


For my understanding, is this(creating code blocks with parantheses) a standard C feature or a gcc extension?

Also wondering if this was intentional, and if yes what was the rationale behind it.

vtikoo · 2020-08-25T22:51:10Z

src/main-oe/sgxlkl_run_oe.c

@@ -1746,6 +1770,10 @@ int main(int argc, char* argv[], char* envp[])
                enclave_image_provided = true;
                strcpy(libsgxlkl, optarg);
                break;
+            case 'i':
+                enclave_image_provided = true;


Suggested change

enclave_image_provided = true;

isolated_image_provided = true;

vtikoo

LGTM. Really appreciate the README, it helped a lot to understand the design!
Had two minor concerns, which should not be PR blocking -

Passing a custom isolated image to sgx-lkl-run-oe seems to be broken. Right now, it will always use libsgxlkl-user.so even if the user specifies an override.
Its not clear to me whether the userspace shared object is signed and whether we are using the signed version.

prp · 2020-09-04T17:19:34Z

Merged as PR #818.

mikbras added 4 commits August 20, 2020 21:54

added userspace isolation

62c0a02

sgx-lkl-musl userspace changes

ff2db6f

remove two syscall bypasses

90ae966

cleanup

cec8b0b

davidchisnall reviewed Aug 24, 2020

View reviewed changes

davidchisnall mentioned this pull request Aug 24, 2020

Dead code in sgx-lkl-musl #816

Open

2 tasks

davidchisnall approved these changes Aug 24, 2020

View reviewed changes

added libsgxlkl-user.so install rule

698355e

letmaik reviewed Aug 24, 2020

View reviewed changes

bodzhang requested a review from vtikoo August 25, 2020 17:35

vtikoo reviewed Aug 25, 2020

View reviewed changes

vtikoo approved these changes Aug 25, 2020

View reviewed changes

Add weak definition for __sgx_init_enclave

d96391e

prp closed this Sep 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User-space separation #815

User-space separation #815

mikbras commented Aug 21, 2020 •

edited

Loading

davidchisnall Aug 24, 2020

davidchisnall Aug 24, 2020

davidchisnall Aug 24, 2020

davidchisnall Aug 24, 2020

davidchisnall Aug 24, 2020

davidchisnall Aug 24, 2020

davidchisnall Aug 24, 2020

davidchisnall Aug 24, 2020

davidchisnall Aug 24, 2020

davidchisnall commented Aug 24, 2020

mikbras commented Aug 24, 2020 •

edited

Loading

davidchisnall commented Aug 24, 2020

letmaik Aug 24, 2020

vtikoo Aug 25, 2020 •

edited

Loading

vtikoo Aug 25, 2020

vtikoo Aug 25, 2020

vtikoo Aug 25, 2020

vtikoo Aug 25, 2020

vtikoo Aug 25, 2020

vtikoo Aug 25, 2020

vtikoo left a comment

prp commented Sep 4, 2020

	args.ua_lkl_syscall = _lkl_syscall_wrapper;
	args.ua_lkl_syscall = lkl_syscall;

		return __sgxlkl_userargs->ua_enclave_mmap(addr, length, mmap_fixed,
		prot, zero_pages);

	return __sgxlkl_userargs->ua_enclave_mmap(addr, length, mmap_fixed,
	prot, zero_pages);
	return mmap(addr, length, prot, MAP_PRIVATE \| MAP_ANONYMOUS \| (mmap_fixed ? MAP_FIXED : 0, -1, 0);

-    return __sgxlkl_userargs->ua_enclave_mmap(addr, length, mmap_fixed,
-        prot, zero_pages);
+    assert(prot == PROT_READ | PROT_WRITE);
+    assert(!mmap_fixed);
+    return mmap(addr, length, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);

		@@ -1391,8 +1405,15 @@ void _create_enclave(

		setting.u.eeid = eeid;

		// Format the follwing path <libsgxlkl>:<libsgxlkl_user>

	// Format the follwing path <libsgxlkl>:<libsgxlkl_user>
	// Format the following path <libsgxlkl>:<libsgxlkl_user>

	enclave_image_provided = true;
	isolated_image_provided = true;

User-space separation #815

User-space separation #815

Conversation

mikbras commented Aug 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidchisnall commented Aug 24, 2020

mikbras commented Aug 24, 2020 • edited Loading

davidchisnall commented Aug 24, 2020

Choose a reason for hiding this comment

vtikoo Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtikoo left a comment

Choose a reason for hiding this comment

prp commented Sep 4, 2020

mikbras commented Aug 21, 2020 •

edited

Loading

mikbras commented Aug 24, 2020 •

edited

Loading

vtikoo Aug 25, 2020 •

edited

Loading