Interrupt enclu execution on main enclave thread exiting #493

maxknv · 2023-07-04T18:55:14Z

Issue SIGUSR1 to worker threads on exiting the main sysloop
Register SIGUSR1 handler to rewrite thread's IP if it's on enclu

Resolves #165

intel-sgx/enclave-runner/src/usercalls/mod.rs

intel-sgx/fortanix-sgx-tools/src/bin/ftxsgx-runner.rs

intel-sgx/enclave-runner/src/usercalls/mod.rs

intel-sgx/fortanix-sgx-tools/src/bin/ftxsgx-runner.rs

raoulstrackx · 2023-07-06T07:42:35Z

intel-sgx/fortanix-sgx-tools/src/bin/ftxsgx-runner.rs

+            });
+            if is_enclu {
+                // At enclu instruction - force IP to the next instruction after enclu
+                unsafe { (*(_context as *mut ucontext_t)).uc_mcontext.gregs[REG_RIP as usize] += 3 }


This causes the enclave execution not to be re-entered, but it does not kill the thread. But what happens in that worker thread in those cases?

some garbage CoResult will be returned and sending it to IO queue will fail as sysloop is terminated by that moment

I think you should adjust RAX and potentially other registers as well to something meaningful

now enclave exiting flag is passed into coenter() to break enclu/AEX loop and special value CoResult::Abort will be returned if an enclave thread was interrupted that way. Why we may still need register adjustments?

Because you need to guarantee the VDSO code branches properly?

intel-sgx/enclave-runner/src/tcs.rs

intel-sgx/fortanix-sgx-tools/src/bin/ftxsgx-runner.rs

jethrogb · 2023-07-11T00:06:02Z

intel-sgx/fortanix-sgx-tools/src/bin/ftxsgx-runner.rs

+            });
+            if is_enclu {
+                // At enclu instruction - force IP to the next instruction after enclu
+                unsafe { (*(_context as *mut ucontext_t)).uc_mcontext.gregs[REG_RIP as usize] += 3 }


I think you should adjust RAX and potentially other registers as well to something meaningful

jethrogb · 2023-07-11T00:07:19Z

intel-sgx/fortanix-sgx-tools/src/bin/ftxsgx-runner.rs

+
+        let hdl = signal::SigHandler::SigAction(handle_sigusr1);
+        let sig_action = signal::SigAction::new(hdl, signal::SaFlags::empty(), signal::SigSet::empty());
+        signal::sigaction(signal::SIGUSR1, &sig_action).unwrap();


USR1 is ok, but isn't e.g. HUP more appropriate? Combined with only conditionally rewriting the IP.

Changed to HUP.
Calling this handler is now conditional for vdso/non-vdso. Is that what you mean by conditional rewriting IP?

intel-sgx/enclave-runner/src/tcs.rs

raoulstrackx · 2023-07-12T16:12:05Z

intel-sgx/enclave-runner/src/tcs.rs

-                sgx_result = run.function;
-                match sgx_result.try_into() {
+                enclu_leaf = run.function;
+                match enclu_leaf.try_into() {
                    Ok(Enclu::EExit) => { /* normal case */ },


This is no longer the "normal case". When you interrupt the enclave in the signal handler, it explicitly writes $EEXIT to enclu_leaf, even though it should be ERESUME. See the vdso. You need to detect such cases and reenter the enclave. You can check the vdso to see which registers you can use for this.

added EXITING flag which is tested in the signup handler addresses this issue as well

jethrogb · 2023-07-12T23:53:07Z

intel-sgx/enclave-runner/src/usercalls/mod.rs

+                // Issuing a signal to return execution control back to the enclave-runner's worker thread.
+                // * In non-vdso case execution control is claimed during AEX using special handler provided by AEP address
+                // in coenter() function and signal is not required
+                unsafe { libc::pthread_kill(handler.as_pthread_t() as _, signal::SIGHUP as _); }


Why is this conditional on has_vdso_sgx_enter_enclave?

there are 2 different solutions for vdso/non-vdso:
with vdso: enclave execution is interrupted by a signal and RIP rewriting.
withouth vdso: enclave execution interrupted using special instruction under AEP, no signal is needed

jethrogb · 2023-07-12T23:58:28Z

intel-sgx/enclave-runner/src/tcs.rs

+                        lea 1f(%rip), %rcx // set SGX AEP
+                        push %rbx          // store original rbx value on the stack
+                        mov {0}, %rbx
+                        enclu
+1:                      pop %rbx           // restore original rbx value in case we interrupt enclave during AEX


I don't understand the change here. This looks like it's intended for RIP rewriting, but the current signal handler is only enabled with the VDSO.

first of all, AEP is changed here to check exiting flag on each AEX and interrupt enclu if needed.
secondly, push/pop instructions for RBX just allow for preserving the original value in RBX.

The comments on %rbx don't add much value. The point here is that due to a limitation of LLVM we can't use rbx as an input to the inline assembly directly, nor can we say it's content is clobbered.

rdx comments removed

intel-sgx/enclave-runner/src/tcs.rs

raoulstrackx · 2023-07-14T14:25:00Z

intel-sgx/enclave-runner/src/tcs.rs

-                sgx_result = run.function;
-                match sgx_result.try_into() {
+                enclu_leaf = run.function;
+                match enclu_leaf.try_into() {
                    Ok(Enclu::EExit) => { /* normal case */ },


Could you also add a comment here that you reach this when the signal handler intervenes and forces the enclave not to be re-entered?

intel-sgx/enclave-runner/src/tcs.rs

raoulstrackx · 2023-07-14T14:52:08Z

intel-sgx/enclave-runner/src/usercalls/mod.rs

+            if has_vdso_sgx_enter_enclave() {
+                // * The enclave thread may be in a long-running `AEX/enclu[ERESUME]` loop.
+                // Issuing a signal to return execution control back to the enclave-runner's worker thread.
+                // * In non-vdso case execution control is claimed during AEX using special handler provided by AEP address


What do you mean with "special handler"?

I fixed the comment

intel-sgx/enclave-runner/src/usercalls/mod.rs

raoulstrackx · 2023-07-14T14:55:15Z

intel-sgx/enclave-runner/src/usercalls/mod.rs

+                // Interrupt enclave execution by setting IP to the instruction following the ENCLU to mimic normal ENCLU[EXIT])
+                unsafe {
+                    (*(_context as *mut ucontext_t)).uc_mcontext.gregs[REG_RIP as usize] += 3;
+                    (*(_context as *mut ucontext_t)).uc_mcontext.gregs[REG_RAX as usize] = 0;


Why 0? This doesn't mimic a normal EEXIT.

I chose 0 just as some deterministic value... Idk which one is better, both looks not very nice. but ok, ill switch to EExit

intel-sgx/enclave-runner/src/usercalls/mod.rs

raoulstrackx · 2023-07-14T15:01:53Z

intel-sgx/enclave-runner/src/usercalls/mod.rs

+            if is_enclu && EXITING.with(|cell| cell.borrow().as_ref().is_some_and(|val| val.load(Ordering::SeqCst) == true)) {
+                // Interrupt enclave execution by setting IP to the instruction following the ENCLU to mimic normal ENCLU[EXIT])
+                unsafe {
+                    (*(_context as *mut ucontext_t)).uc_mcontext.gregs[REG_RIP as usize] += 3;


Can you also rename _context to context?

intel-sgx/enclave-runner/src/tcs.rs

intel-sgx/enclave-runner/src/usercalls/mod.rs

raoulstrackx · 2023-07-19T07:28:18Z

Looks good to me! @mkaynov can you rebase so I can take the final version for a spin?

intel-sgx/enclave-runner/src/usercalls/mod.rs

maxknv requested review from vn971, raoulstrackx and jethrogb July 5, 2023 07:34

maxknv force-pushed the max/gh-165-abort-hanged-enclave-thread branch from 8226083 to a1a2e0f Compare July 5, 2023 10:52

vn971 reviewed Jul 6, 2023

View reviewed changes

intel-sgx/enclave-runner/src/usercalls/mod.rs Outdated Show resolved Hide resolved

vn971 reviewed Jul 6, 2023

View reviewed changes

intel-sgx/fortanix-sgx-tools/src/bin/ftxsgx-runner.rs Outdated Show resolved Hide resolved

vn971 reviewed Jul 6, 2023

View reviewed changes

intel-sgx/fortanix-sgx-tools/src/bin/ftxsgx-runner.rs Outdated Show resolved Hide resolved

vn971 reviewed Jul 6, 2023

View reviewed changes

intel-sgx/fortanix-sgx-tools/src/bin/ftxsgx-runner.rs Outdated Show resolved Hide resolved

raoulstrackx reviewed Jul 6, 2023

View reviewed changes

maxknv force-pushed the max/gh-165-abort-hanged-enclave-thread branch 4 times, most recently from 739418a to a501d3e Compare July 10, 2023 08:28

maxknv changed the title ~~#165 Abort hanged enclave thread by SIGUSR1~~ Abort hanged enclave thread by SIGUSR1 Jul 10, 2023

maxknv requested a review from raoulstrackx July 10, 2023 08:39

raoulstrackx reviewed Jul 10, 2023

View reviewed changes

intel-sgx/enclave-runner/src/tcs.rs Outdated Show resolved Hide resolved

intel-sgx/enclave-runner/src/tcs.rs Outdated Show resolved Hide resolved

raoulstrackx reviewed Jul 10, 2023

View reviewed changes

intel-sgx/enclave-runner/src/tcs.rs Outdated Show resolved Hide resolved

intel-sgx/enclave-runner/src/tcs.rs Show resolved Hide resolved

jethrogb reviewed Jul 11, 2023

View reviewed changes

maxknv requested review from raoulstrackx and jethrogb July 11, 2023 11:49

raoulstrackx reviewed Jul 12, 2023

View reviewed changes

intel-sgx/enclave-runner/src/tcs.rs Outdated Show resolved Hide resolved

maxknv force-pushed the max/gh-165-abort-hanged-enclave-thread branch from f0a9c61 to 5d62085 Compare July 12, 2023 16:05

raoulstrackx reviewed Jul 12, 2023

View reviewed changes

jethrogb reviewed Jul 12, 2023

View reviewed changes

maxknv changed the title ~~Abort hanged enclave thread by SIGUSR1~~ Interrupt enclu execution on main enclave thread exiting Jul 14, 2023

maxknv requested review from jethrogb and raoulstrackx July 14, 2023 11:00

raoulstrackx reviewed Jul 14, 2023

View reviewed changes

maxknv force-pushed the max/gh-165-abort-hanged-enclave-thread branch 2 times, most recently from 3b3ad1a to cd0790f Compare July 17, 2023 09:49

raoulstrackx reviewed Jul 18, 2023

View reviewed changes

maxknv force-pushed the max/gh-165-abort-hanged-enclave-thread branch 4 times, most recently from 8268018 to bbe7c37 Compare July 25, 2023 14:25

Interrupt unfinished Enclu on main enclave thread exit

9d998d9

maxknv force-pushed the max/gh-165-abort-hanged-enclave-thread branch from bbe7c37 to 9d998d9 Compare July 26, 2023 10:46

raoulstrackx added 2 commits August 2, 2023 11:31

Making sighup handler signal safe

8da26da

Cleaning up

caee4f8

raoulstrackx reviewed Aug 2, 2023

View reviewed changes

intel-sgx/enclave-runner/src/usercalls/mod.rs Outdated Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interrupt enclu execution on main enclave thread exiting #493

Interrupt enclu execution on main enclave thread exiting #493

maxknv commented Jul 4, 2023 •

edited

raoulstrackx Jul 6, 2023

maxknv Jul 6, 2023

jethrogb Jul 11, 2023

maxknv Jul 11, 2023

jethrogb Jul 11, 2023

jethrogb Jul 11, 2023

jethrogb Jul 11, 2023

maxknv Jul 11, 2023

raoulstrackx Jul 12, 2023

maxknv Jul 14, 2023

jethrogb Jul 12, 2023

maxknv Jul 13, 2023

jethrogb Jul 12, 2023

maxknv Jul 13, 2023

raoulstrackx Jul 14, 2023

maxknv Jul 17, 2023

raoulstrackx Jul 14, 2023

raoulstrackx Jul 14, 2023

maxknv Jul 17, 2023

raoulstrackx Jul 14, 2023

maxknv Jul 17, 2023

raoulstrackx Jul 14, 2023

maxknv Jul 17, 2023

raoulstrackx commented Jul 19, 2023

Interrupt enclu execution on main enclave thread exiting #493

Are you sure you want to change the base?

Interrupt enclu execution on main enclave thread exiting #493

Conversation

maxknv commented Jul 4, 2023 • edited

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

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

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

raoulstrackx commented Jul 19, 2023

maxknv commented Jul 4, 2023 •

edited