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 v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions
From:   Andy Lutomirski <luto@amacapital.net>
In-Reply-To: <20181207200935.GE10404@linux.intel.com>
Date:   Fri, 7 Dec 2018 12:16:59 -0800
Cc:     Andy Lutomirski <luto@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        X86 ML <x86@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        "H. Peter Anvin" <hpa@zytor.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
        Josh Triplett <josh@joshtriplett.org>,
        linux-sgx@vger.kernel.org, haitao.huang@linux.intel.com,
        Jethro Beekman <jethro@fortanix.com>,
        "Dr. Greg Wettstein" <greg@enjellic.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <4CEB5945-9562-40FA-8CCA-A1675D55B001@amacapital.net>
References: <20181206221922.31012-1-sean.j.christopherson@intel.com> <20181206221922.31012-5-sean.j.christopherson@intel.com> <CALCETrXRJ645=08fyeoMQ949fLB1TvhsgERFVx5mAHdViEjq8Q@mail.gmail.com> <20181207165145.GB10404@linux.intel.com> <CALCETrVkEW82PzQZhJ5iS849DLY1gbd8YjH5F7P1J7hgjkjiOQ@mail.gmail.com> <20181207190257.GC10404@linux.intel.com> <EFA99261-BE93-49DA-9A8F-E6FD28069AFC@amacapital.net> <20181207200935.GE10404@linux.intel.com>
To:     Sean Christopherson <sean.j.christopherson@intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Dec 7, 2018, at 12:09 PM, Sean Christopherson <sean.j.christopherson@in=
tel.com> wrote:
>=20
>> On Fri, Dec 07, 2018 at 11:23:10AM -0800, Andy Lutomirski wrote:
>>=20
>>>> On Dec 7, 2018, at 11:02 AM, Sean Christopherson <sean.j.christopherson=
@intel.com> wrote:
>>>>=20
>>>> On Fri, Dec 07, 2018 at 09:56:09AM -0800, Andy Lutomirski wrote:
>>>> On Fri, Dec 7, 2018 at 8:51 AM Sean Christopherson
>>>> <sean.j.christopherson@intel.com> wrote:
>>>>> I like that the exit handler allows userspace to trap/panic with the f=
ull
>>>>> call stack in place, and in a dedicated path, i.e. outside of the basi=
c
>>>>> enter/exit code.  An exit handler probably doesn't fundamentally chang=
e
>>>>> what userspace can do with respect to debugging/reporting, but I think=

>>>>> it would actually simplify some userspace implementations, e.g. I'd us=
e
>>>>> it in my tests like so:
>>>>>=20
>>>>> long fault_handler(struct sgx_enclave_exit_info *exit_info, void *tcs,=
 void *priv)
>>>>> {
>>>>>       if (exit_info->leaf =3D=3D SGX_EEXIT)
>>>>>               return 0;
>>>>>=20
>>>>>       <report exception and die/hang>
>>>>> }
>>>>>=20
>>>>=20
>>>> Hmm.  That't not totally silly, although you could accomplish almost
>>>> the same thing by wrapping the vDSO helper in another function.
>>>=20
>>> True, but I think there's value in having the option to intercept an
>>> exception at the exact(ish) point of failure, without having to return
>>> up the stack.
>>>=20
>>> The enclave has full access to the process' memory space, including the
>>> untrsuted stack.  It's not too far fetched to envision a scenario where
>>> the enclave corrupts the stack even if the enclave isn't intentionally
>>> using the stack, e.g. the host passes in variable that a points at the
>>> stack instead of whatever memory is supposed to be shared between the
>>> enclave and host.  It'd be nice to have the ability to triage something
>>> like that without having to e.g. configure breakpoints on the stack.
>>=20
>> Ah, I see. You=E2=80=99re saying that, if the non-enclave stare is corrup=
ted such
>> that RIP  is okay and RSP still points somewhere reasonable but the retur=
n
>> address is garbage, then we can at least get to the fault handler and pri=
nt
>> something?
>=20
> Yep.  Even for something more subtle like GPR corruption it could dump the=

> entire call stack before attempting to return back up.
>=20
>> This only works if the fault handler pointer itself is okay, though, whic=
h
>> somewhat limits the usefulness, given that its pointer is quite likely to=

>> be on the stack very close to the return address.
>=20
> Yeah, it's not a silver bullet by any means, but it does seem useful for a=
t
> least some scenarios.  Even exploding when invoking the handler instead of=

> at a random point might prove useful, e.g. "calling my exit handler explod=
ed,
> maybe my enclave corrupted the stack!".

Here=E2=80=99s another idea: calculate some little hash or other checksum of=
 RSP, RBP, and perhaps a couple words on the stack, and do:

call __vdso_enclave_corrupted_state

If you get a mismatch after return. That function could be:

call __vdso_enclave_corrupted_state:
  ud2

And now the debug trace makes it very clear what happened.

This may or may not be worth the effort. But ISTM the enclave is almost as l=
ikely to corrupt the host state and the. EEXIT as it is to corrupt the host s=
tate and then fault.

BTW, I read through Fortanix=E2=80=99s documentation of the Windows SGX call=
ing conventions, and it seems to want RSI and RDI as out params.  Letting th=
e vDSO be used to invoke Windows-targeted enclaves seems useful.

>=20
>> I really wish the ENCLU instruction had seen fit to preserve some registe=
rs.
>=20
> Speaking of preserving registers, the asm blob needs to mark RBX as
> clobbered since it's modified for EEXIT.

Have fun with that.  The x86_32 compiler seems to really like having its PIC=
 register preserved, and you may get some lovely compiler errors.=