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.

Sign up for GitHub

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

Jump to bottom

Enclave threads should be terminated on exit #165

Open
jethrogb opened this issue Aug 5, 2019 · 5 comments · May be fixed by #493
Open

Enclave threads should be terminated on exit #165

jethrogb opened this issue Aug 5, 2019 · 5 comments · May be fixed by #493
Assignees
@maxknv
Labels
bug C-enclave-runner Crate: enclave-runner
Milestone
Seeds

Comments

@jethrogb
Copy link
Member

jethrogb commented Aug 5, 2019 •
edited

This program may not terminate:

use std::{sync::mpsc, thread};

fn main() {
    let (tx, rx) = mpsc::channel();
    thread::spawn(move || {
        tx.send(()).unwrap();
        loop {}
    });
    rx.recv().unwrap();
    println!("Exiting main thread");
}

To fix:

  1. Change the AEP so that it won't automatically re-enter the enclave after AEX.
  2. Add a way to induce AEX (e.g. pthread_kill)
  3. Add a way to return EnclaveAbort or such from coenter when another thread does the exit usercall.
@jethrogb jethrogb added C-enclave-runner Crate: enclave-runner enhancement bug and removed enhancement labels Aug 5, 2019
@jethrogb
Copy link
Member Author

jethrogb commented Jun 5, 2023 •
edited

Change the AEP so that it won't automatically re-enter the enclave after AEX.

Hope this is possible with VDSO?

@raoulstrackx raoulstrackx mentioned this issue Jun 15, 2023
Open
@raoulstrackx
Copy link
Contributor

Unfortunately, the user_handler in the SgxEnclaveRun of the vdso isn't called upon each enclu[eresume], nor when a signal is send to the thread. See this poc.

@jethrogb
Copy link
Member Author

That's unfortunate, I do remember some discussion about this on LKML but not the resolution.

Your POC seems unnecessarily complex. Why don't you just pass sgx_user_handler directly in SgxEnclaveRun?

Anyway, that means step 1 should be changed to use a signal handler instead of a different AEP.

@raoulstrackx
Copy link
Contributor

raoulstrackx commented Jun 22, 2023 •
edited

Your POC seems unnecessarily complex. Why don't you just pass sgx_user_handler directly in SgxEnclaveRun?

Yes that's the first thing I tried. However, he vdso always calls the sgx_user_handler when the enclave exits. By the time it returns safe-by-caller registers may already have changed, but our code uses those registers to communicate with the enclave. The easiest option for this poc (without relying on compiler assumptions) was to wrap the caller in a code block that saves these values before calling the sgx_user_handler.

Anyway, that means step 1 should be changed to use a signal handler instead of a different AEP.

Yes I've developed a POC for that as well

maxknv pushed a commit that referenced this issue Jun 22, 2023
Modify AEP for AEX to exit hanged enclave thread #165
e1a9f3e
maxknv pushed a commit that referenced this issue Jun 23, 2023
Abort hanged enclave threads #165. Non-vdso.
60cb1de
@maxknv
Copy link

maxknv commented Jun 26, 2023

@raoulstrackx @jethrogb
I made an implementation that allows interrupting the hanged enclave thread by rewriting AEP, despite some open TODOs in the code this technique is proven to work, though only for non-vdso case.

For the VDSO none of these things are working :

  • AEP rewriting: not possible
  • Signal handler: got executed by the main thread regardless of the thread id signal being issued to
  • custom sgx_enclave_user_handler_t: it is not triggered by AEX at all
    Are there any other ideas in mind to make use of?

maxknv pushed a commit that referenced this issue Jul 4, 2023
#165 Abort hanged enclave thread by SIGUSR1
51ef300
@maxknv maxknv linked a pull request Jul 4, 2023 that will close this issue
Open
maxknv pushed a commit that referenced this issue Jul 10, 2023
Merge branch 'max/gh-165-abort-hanged-enclave-thread' of https://gith…
da6a1ab 
…ub.com/fortanix/rust-sgx into max/gh-165-abort-hanged-enclave-thread
@BelalH BelalH added this to the Seeds milestone Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@maxknv maxknv

Labels
bug C-enclave-runner Crate: enclave-runner
Projects
None yet
Milestone
Seeds
Development

Successfully merging a pull request may close this issue.

Interrupt enclu execution on main enclave thread exiting fortanix/rust-sgx
4 participants
@jethrogb @maxknv @BelalH @raoulstrackx