Re: [RFC PATCH 4/4] x86/vdso: x86/sgx: Allow the user to exit the vDSO loop on interrupts

[Jethro Beekman <jethro@fortanix.com>]

[-- Attachment #1: Type: text/plain, Size: 5082 bytes --]

On 2020-08-19 17:02, Andy Lutomirski wrote:
> On Wed, Aug 19, 2020 at 7:21 AM Jethro Beekman <jethro@fortanix.com> wrote:
>>
>> On 2020-08-18 19:15, Andy Lutomirski wrote:
>>> On Mon, Aug 17, 2020 at 9:24 PM Sean Christopherson
>>> <sean.j.christopherson@intel.com> wrote:
>>>>
>>>> Allow userspace to exit the vDSO on interrupts that are acknowledged
>>>> while the enclave is active.  This allows the user's runtime to switch
>>>> contexts at opportune times without additional overhead, e.g. when using
>>>> an M:N threading model (where M user threads run N TCSs, with N > M).
>>>
>>> This is IMO rather odd.  We don't support this type of notification on
>>> interrupts for normal user code.  The fact user code can detect
>>> interrupts during enclave execution is IMO an oddity of SGX, and I
>>> have asked Intel to consider replacing the AEX mechanism with
>>> something more transparent to user mode.  If this ever happens, this
>>> mechanism is toast.
>>
>> Let's design the current interface for the current architecture. We can deal with a new architecture if and when Intel provides it.
> 
> No.  If Intel fixes the architecture, the proposed interface will
> *stop working*.
> 
>>
>>> Even without architecture changes, building a *reliable* M:N threading
>>> mechanism on top of this will be difficult or impossible, as there is
>>> no particular guarantee that a thread will get timing interrupts at> all or that these interrupts will get lucky and hit enclave code, thus
>>> triggering an AEX.  We certainly don't, and probably never will,
>>> support any corresponding feature for non-enclave code.
>>
>> There's no guarantee, but this vDSO exit mechanism is a prerequisite. Both for context switching and aborting an enclave, userspace *must* have a way to trigger exit from enclave mode *and* recover the user stack in a sane manner. Userspace *should* also be able to do this in a way that's compatible with library use, so calling timer_create or pthread_kill to deliver a signal would be ok, but installing a signal handler should be avoided (the whole reason behind this vDSO call).
> 
> If you want to abort an enclave, abort it the same way you abort any
> other user code -- send a signal or something.  If something is wrong> with signal handling in the proposed vDSO interface, then by all
> means, let's fix it.  If we need better library signal support, let's
> add such a thing.  If you really want to abort an enclave *cleanly*
> without any chance of garbage left around, stick it in a subprocess.
> We are not playing the game where someone sets a flag, crosses their
> fingers, and waits for an interrupt.

Sending a signal is not sufficient. The current __vdso_sgx_enter_enclave call is not interruptible.

> 
>>
>>> So this seems like an odd, and possibly unsupportable, feature to add.
>>
>> I can implement all this without the vDSO call today, so why not support it? That just means not everyone is going to use the vDSO call, again resulting in potential problems when multiple libraries want to use enclaves.
> 
> No offense, but if you want to write garbage software with entirely
> user code, I can't stop you, but the kernel is not going to give this
> anything resembling a stamp of approval.  When you inevitably discover
> that it has broken for one reason or another and your software stack
> stops making forward progress, I don't want anyone playing the "ABI
> stability" card and asking the upstream kernel to work around the
> breakage.
> 
> Now maybe I'm wrong and there's an actual legitimate use case for this
> trick, in which case I'd like to be enlightened.  But M:N threading
> and enclave aborting don't sound like legitimate use cases.
> 

Why is it ok for this code to return to userspace after 1 second:



void ignore_signal_but_dont_restart(int s) {}

// set SIGALRM handler if none set (FIXME: racy)
struct sigaction sa_old;
struct sigaction sa = {
    .sa_handler = ignore_signal_but_dont_restart,
    .sa_flags = 0,
};
sigaction(SIGALRM, &sa, &sa_old);
if (!(sa_old.sa_handler == SIG_IGN || sa_old.sa_handler == SIG_DFL)
    || (sa_old.sa_flags & SA_SIGINFO)) {
    sa_old.sa_flags &= ~SA_RESTART;
    sigaction(SIGALRM, &sa_old, NULL);
}

alarm(1);

char buf;
read(0, &buf, 1);



But, according to your train of thought, this code must hang indefinitely (assuming the enclave is not calling EEXIT)?



void ignore_signal_but_dont_restart(int s) {}

// set SIGALRM handler if none set (FIXME: racy)
struct sigaction sa_old;
struct sigaction sa = {
    .sa_handler = ignore_signal_but_dont_restart,
    .sa_flags = 0,
};
sigaction(SIGALRM, &sa, &sa_old);
if (!(sa_old.sa_handler == SIG_IGN || sa_old.sa_handler == SIG_DFL)
    || (sa_old.sa_flags & SA_SIGINFO)) {
    sa_old.sa_flags &= ~SA_RESTART;
    sigaction(SIGALRM, &sa_old, NULL);
}

alarm(1);

__vdso_sgx_enter_enclave(0, 0, 0, 2, 0, 0, tcs, NULL, NULL);



Tested on Linux 5.9-rc1 with SGX patches v36.

--
Jethro Beekman | Fortanix


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4054 bytes --]