Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (1.0)
Subject: Re: [RFC PATCH v4 5/5] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions
From:   Andy Lutomirski <luto@amacapital.net>
In-Reply-To: <20181214170310.GC22063@linux.intel.com>
Date:   Fri, 14 Dec 2018 10:44:10 -0800
Cc:     Jethro Beekman <jethro@fortanix.com>,
        Andy Lutomirski <luto@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        "x86@kernel.org" <x86@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
        Josh Triplett <josh@joshtriplett.org>,
        Haitao Huang <haitao.huang@linux.intel.com>,
        "Dr . Greg Wettstein" <greg@enjellic.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <90D05734-1583-4306-A9A4-18E4A1390F3B@amacapital.net>
References: <20181213213135.12913-1-sean.j.christopherson@intel.com> <20181213213135.12913-6-sean.j.christopherson@intel.com> <cda13cff-1a56-a40f-7d69-f0f1ab752f8e@fortanix.com> <20181214151204.GA22063@linux.intel.com> <20181214153830.GB22063@linux.intel.com> <20181214170310.GC22063@linux.intel.com>
To:     Sean Christopherson <sean.j.christopherson@intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Dec 14, 2018, at 9:03 AM, Sean Christopherson <sean.j.christopherson@in=
tel.com> wrote:
>=20
>> On Fri, Dec 14, 2018 at 07:38:30AM -0800, Sean Christopherson wrote:
>>> On Fri, Dec 14, 2018 at 07:12:04AM -0800, Sean Christopherson wrote:
>>>> On Fri, Dec 14, 2018 at 09:55:49AM +0000, Jethro Beekman wrote:
>>>>> On 2018-12-14 03:01, Sean Christopherson wrote:
>>>>> +2:    pop    %rbx
>>>>> +    pop    %r12
>>>>> +    pop    %r13
>>>>> +    pop    %r14
>>>>> +    pop    %r15
>>>>> +    pop    %rbp
>>>>> +    ret
>>>>=20
>>>> x86-64 ABI requires that you call CLD here (enclave may set it).
>>>=20
>>> Ugh.  Technically MXCSR and the x87 CW also need to be preserved.
>>>=20
>>> What if rather than treating the enclave as hostile we require it to be
>>> compliant with the x86-64 ABI like any other function?  That would solve=

>>> the EFLAGS.DF, MXCSR and x87 issues without adding unnecessary overhead.=

>>> And we wouldn't have to save/restore R12-R15.  It'd mean we couldn't use=

>>> the stack's red zone to hold @regs and @e, but that's poor form anyways.=

>>=20
>> Grr, except the processor crushes R12-R15, FCW and MXCSR on asynchronous
>> exits.  But not EFLAGS.DF, that's real helpful.
>=20
> I can think of three options that are at least somewhat reasonable:
>=20
>  1) Save/restore MXCSR and FCW
>=20
>     + 100% compliant with the x86-64 ABI
>     + Callable from any code
>     + Minimal documentation required
>     - Restoring MXCSR/FCW is likely unnecessary 99% of the time
>     - Slow
>=20
>  2) Clear EFLAGS.DF but not save/restore MXCSR and FCW
>=20
>     + Mostly compliant with the x86-64 ABI
>     + Callable from any code that doesn't use SIMD registers
>     - Need to document deviations from x86-64 ABI
>=20
>  3) Require the caller to save/restore everything.
>=20
>     + Fast
>     + Userspace can pass all GPRs to the enclave (minus EAX, RBX and RCX)
>     - Completely custom ABI
>     - For all intents and purposes must be called from an assembly wrapper=

>=20
> Option (3) actually isn't all that awful.  RCX can be used to pass an
> optional pointer to a 'struct sgx_enclave_exception' and we can still
> return standard error codes, e.g. -EFAULT.

I like 3, but:

>=20
> E.g.:
>=20
> /**
> * __vdso_sgx_enter_enclave() - Enter an SGX enclave
> *
> * %eax:    ENCLU leaf, must be EENTER or ERESUME
> * %rbx:    TCS, must be non-NULL
> * %rcx:    Optional pointer to 'struct sgx_enclave_exception'
> *
> * Return:
> *  0 on a clean entry/exit to/from the enclave
> *  -EINVAL if ENCLU leaf is not allowed or if TCS is NULL
> *  -EFAULT if ENCLU or the enclave faults
> */
> ENTRY(__vdso_sgx_enter_enclave)
>    /* EENTER <=3D leaf <=3D ERESUME */
>    cmp    $0x2, %eax
>    jb    bad_input
>=20
>    cmp    $0x3, %eax
>    ja    bad_input
>=20
>    /* TCS must be non-NULL */
>    test    %rbx, %rbx
>    je    bad_input
>=20
>    /* save @exception pointer */
>    push    %rcx
>=20
>    /* load leaf, TCS and AEP for ENCLU */
>    lea    1f(%rip),  %rcx
> 1:    enclu
>=20
>    add    0x8, %rsp
>    xor    %eax, %eax
>    ret
>=20
> bad_input:
>    mov     $(-EINVAL), %rax
>    ret
>=20
> .pushsection .fixup, "ax"
> 2:    pop    %rcx   =20
>    test    %rcx, %rcx
>    je    3f
>=20
>    mov    %eax, EX_LEAF(%rcx)
>    mov    %di,  EX_TRAPNR(%rcx)
>    mov    %si,  EX_ERROR_CODE(%rcx)
>    mov    %rdx, EX_ADDRESS(%rcx)
> 3:    mov     $(-EFAULT), %rax
>    ret

I=E2=80=99m not totally sold on -EFAULT as the error code.  That usually ind=
icates a bad pointer.  I=E2=80=99m not sure I have a better suggestion.

> .popsection
>=20
> _ASM_VDSO_EXTABLE_HANDLE(1b, 3b)
>=20
> ENDPROC(__vdso_sgx_enter_enclave)