Linux userland API discussions
 help / color / mirror / Atom feed
* Re: RFC: userspace exception fixups
From: Jann Horn @ 2018-11-02 23:36 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: sean.j.christopherson, Dave Hansen, Linus Torvalds, dalias,
	Dave Hansen, jethro, jarkko.sakkinen, Florian Weimer, Linux API,
	the arch/x86 maintainers, linux-arch, kernel list, Peter Zijlstra,
	nhorman, npmccallum, serge.ayoun, shay.katz-zamir, linux-sgx,
	andriy.shevchenko, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	carlos, adhemerval.zane
In-Reply-To: <CALCETrXq1Kj=McxnPVjkscNq-b_7LeoFOdu7qKjHdvwcfFh9Og@mail.gmail.com>

On Sat, Nov 3, 2018 at 12:32 AM Andy Lutomirski <luto@kernel.org> wrote:
> On Fri, Nov 2, 2018 at 4:28 PM Jann Horn <jannh@google.com> wrote:
> > On Fri, Nov 2, 2018 at 11:04 PM Sean Christopherson
> > <sean.j.christopherson@intel.com> wrote:
> > > On Fri, Nov 02, 2018 at 08:02:23PM +0100, Jann Horn wrote:
> > > > On Fri, Nov 2, 2018 at 7:27 PM Sean Christopherson
> > > > <sean.j.christopherson@intel.com> wrote:
> > > > > On Fri, Nov 02, 2018 at 10:48:38AM -0700, Andy Lutomirski wrote:
> > > > > > This whole mechanism seems very complicated, and it's not clear
> > > > > > exactly what behavior user code wants.
> > > > >
> > > > > No argument there.  That's why I like the approach of dumping the
> > > > > exception to userspace without trying to do anything intelligent in
> > > > > the kernel.  Userspace can then do whatever it wants AND we don't
> > > > > have to worry about mucking with stacks.
> > > > >
> > > > > One of the hiccups with the VDSO approach is that the enclave may
> > > > > want to use the untrusted stack, i.e. the stack that has the VDSO's
> > > > > stack frame.  For example, Intel's SDK uses the untrusted stack to
> > > > > pass parameters for EEXIT, which means an AEX might occur with what
> > > > > is effectively a bad stack from the VDSO's perspective.
> > > >
> > > > What exactly does "uses the untrusted stack to pass parameters for
> > > > EEXIT" mean? I guess you're saying that the enclave is writing to
> > > > RSP+[0...some_positive_offset], and the written data needs to be
> > > > visible to the code outside the enclave afterwards?
> > >
> > > As is, they actually do it the other way around, i.e. negative offsets
> > > relative to the untrusted %RSP.  Going into the enclave there is no
> > > reserved space on the stack.  The SDK uses EEXIT like a function call,
> > > i.e. pushing parameters on the stack and making an call outside of the
> > > enclave, hence the name out-call.  This allows the SDK to handle any
> > > reasonable out-call without a priori knowledge of the application's
> > > maximum out-call "size".
> >
> > But presumably this is bounded to be at most 128 bytes (the red zone
> > size), right? Otherwise this would be incompatible with
> > non-sigaltstack signal delivery.
>
>
> I think Sean is saying that the enclave also updates RSP.

Ah, bleh, of course.

^ permalink raw reply

* Re: [PATCH -next v2 0/3] sysvipc: introduce STAT_ANY commands
From: Michael Kerrisk (man-pages) @ 2018-11-04 16:29 UTC (permalink / raw)
  To: Davidlohr Bueso, akpm
  Cc: mtk.manpages, mhocko, robert.kettler, manfred, ebiederm, keescook,
	linux-api, linux-kernel, Joe Lawrence
In-Reply-To: <20180320185503.nimh3htpprbg7nth@linux-n805>

Hello Davidlohr,

On 3/20/18 7:55 PM, Davidlohr Bueso wrote:
> On Thu, 15 Feb 2018, Davidlohr Bueso wrote:
> 
>> Once (if) merged, I will submit the necesary manpage updates. But I'm
>> thinking something like:
> 
> Hi Michael, here are the updated manpage entries. As always, please feel
> free to modify the descriptions as you see fit.

Thanks. Patch applied. Sorry for the delay...

Cheers,

Michael

> ------------8<----------------------------------------------------------
> [PATCH] sysvipc: add *_STAT_ANY command descriptions
> 
> The *ctl syscall descriptions have been updated to reflect the
> new commands in msg queues, semaphores and shmem.
> 
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
>  man2/msgctl.2 | 20 +++++++++++++++++---
>  man2/semctl.2 | 19 ++++++++++++++++++-
>  man2/shmctl.2 | 17 +++++++++++++++--
>  3 files changed, 50 insertions(+), 6 deletions(-)
> 
> diff --git a/man2/msgctl.2 b/man2/msgctl.2
> index df74c750457a..31b1a1f12fb0 100644
> --- a/man2/msgctl.2
> +++ b/man2/msgctl.2
> @@ -33,6 +33,7 @@
>  .\"	Language and formatting clean-ups
>  .\"	Added msqid_ds and ipc_perm structure definitions
>  .\" 2005-08-02, mtk: Added IPC_INFO, MSG_INFO, MSG_STAT descriptions
> +.\" 2018-03-20, dbueso: Added MSG_STAT_ANY description.
>  .\"
>  .TH MSGCTL 2 2017-09-15 "Linux" "Linux Programmer's Manual"
>  .SH NAME
> @@ -222,10 +223,23 @@ Return a
>  structure as for
>  .BR IPC_STAT .
>  However, the
> -.I msqid
> +.I msgid
>  argument is not a queue identifier, but instead an index into
>  the kernel's internal array that maintains information about
>  all message queues on the system.
> +.TP
> +.BR MSG_STAT_ANY " (Linux-specific)"
> +Return a
> +.I msqid_ds
> +structure as for
> +.BR MSG_STAT .
> +However, the
> +.I msg_perm.mode
> +is not checked for read access for
> +.IR msqid ,
> +resembing the behaviour of
> +/proc/sysvipc/msg.
> +.PP
>  .SH RETURN VALUE
>  On success,
>  .BR IPC_STAT ,
> @@ -241,10 +255,10 @@ operation returns the index of the highest used entry in the
>  kernel's internal array recording information about all
>  message queues.
>  (This information can be used with repeated
> -.B MSG_STAT
> +.B MSG_STAT or MSG_STAT_ANY
>  operations to obtain information about all queues on the system.)
>  A successful
> -.B MSG_STAT
> +.B MSG_STAT or MSG_STAT_ANY
>  operation returns the identifier of the queue whose index was given in
>  .IR msqid .
>  .PP
> diff --git a/man2/semctl.2 b/man2/semctl.2
> index 02930d2c74ae..0ebe0434c05f 100644
> --- a/man2/semctl.2
> +++ b/man2/semctl.2
> @@ -37,6 +37,7 @@
>  .\"	Rewrote semun text
>  .\"	Added semid_ds and ipc_perm structure definitions
>  .\" 2005-08-02, mtk: Added IPC_INFO, SEM_INFO, SEM_STAT descriptions.
> +.\" 2018-03-20, dbueso: Added SEM_STAT_ANY description.
>  .\"
>  .TH SEMCTL 2 2017-09-15 "Linux" "Linux Programmer's Manual"
>  .SH NAME
> @@ -240,6 +241,17 @@ argument is not a semaphore identifier, but instead an index into
>  the kernel's internal array that maintains information about
>  all semaphore sets on the system.
>  .TP
> +.BR SEM_STAT_ANY " (Linux-specific)"
> +Return a
> +.I seminfo
> +structure containing the same information as for
> +.BR SEM_STAT .
> +However, the
> +.I sem_perm.mode
> +is not checked for read access for
> +.IR semid ,
> +resembing the behaviour of
> +/proc/sysvipc/sem.
>  .B GETALL
>  Return
>  .B semval
> @@ -367,7 +379,7 @@ the index of the highest used entry in the
>  kernel's internal array recording information about all
>  semaphore sets.
>  (This information can be used with repeated
> -.B SEM_STAT
> +.B SEM_STAT or SEM_STAT_ANY
>  operations to obtain information about all semaphore sets on the system.)
>  .TP
>  .B SEM_INFO
> @@ -377,6 +389,10 @@ as for
>  .B SEM_STAT
>  the identifier of the semaphore set whose index was given in
>  .IR semid .
> +.TP
> +.B SEM_STAT_ANY
> +as for
> +.BR SEM_STAT .
>  .PP
>  All other
>  .I cmd
> @@ -397,6 +413,7 @@ has one of the values
>  .BR GETZCNT ,
>  .BR IPC_STAT ,
>  .BR SEM_STAT ,
> +.BR SEM_STAT_ANY ,
>  .BR SETALL ,
>  or
>  .B SETVAL
> diff --git a/man2/shmctl.2 b/man2/shmctl.2
> index 7bb503999941..42c47d9f4350 100644
> --- a/man2/shmctl.2
> +++ b/man2/shmctl.2
> @@ -41,6 +41,7 @@
>  .\" 2005-04-25, mtk -- noted aberrant Linux behavior w.r.t. new
>  .\"	attaches to a segment that has already been marked for deletion.
>  .\" 2005-08-02, mtk: Added IPC_INFO, SHM_INFO, SHM_STAT descriptions.
> +.\" 2018-03-20, dbueso: Added SHM_STAT_ANY description.
>  .\"
>  .TH SHMCTL 2 2017-09-15 "Linux" "Linux Programmer's Manual"
>  .SH NAME
> @@ -242,6 +243,18 @@ However, the
>  argument is not a segment identifier, but instead an index into
>  the kernel's internal array that maintains information about
>  all shared memory segments on the system.
> +.TP
> +.BR SHM_STAT_ANY " (Linux-specific)"
> +Return a
> +.I shmid_ds
> +structure as for
> +.BR SHM_STAT .
> +However, the
> +.I shm_perm.mode
> +is not checked for read access for
> +.IR shmid ,
> +resembing the behaviour of
> +/proc/sysvipc/shm.
>  .PP
>  The caller can prevent or allow swapping of a shared
>  memory segment with the following \fIcmd\fP values:
> @@ -287,7 +300,7 @@ operation returns the index of the highest used entry in the
>  kernel's internal array recording information about all
>  shared memory segments.
>  (This information can be used with repeated
> -.B SHM_STAT
> +.B SHM_STAT or SHM_STAT_ANY
>  operations to obtain information about all shared memory segments
>  on the system.)
>  A successful
> @@ -328,7 +341,7 @@ isn't accessible.
>  \fIshmid\fP is not a valid identifier, or \fIcmd\fP
>  is not a valid command.
>  Or: for a
> -.B SHM_STAT
> +.B SHM_STAT or SHM_STAT_ANY
>  operation, the index value specified in
>  .I shmid
>  referred to an array slot that is currently unused.
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

^ permalink raw reply

* Re: [PATCH v2 5/5] nfs: don't clear STATX_ATIME from result_mask
From: Andreas Dilger @ 2018-11-05 22:29 UTC (permalink / raw)
  To: Trond Myklebust
  Cc: miklos@szeredi.hu, linux-kernel@vger.kernel.org,
	amir73il@gmail.com, mszeredi@redhat.com,
	linux-api@vger.kernel.org, dhowells@redhat.com, fw@deneb.enyo.de,
	mtk.manpages@gmail.com, linux-fsdevel@vger.kernel.org
In-Reply-To: <fbc3501c07fd48e65c0ffc8634870bf57a6f7fe1.camel@hammerspace.com>

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

On Oct 20, 2018, at 11:46 AM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> 
> On Fri, 2018-10-19 at 22:48 +0200, Miklos Szeredi wrote:
>> On Fri, Oct 19, 2018 at 8:14 PM, Trond Myklebust
>> <trondmy@hammerspace.com> wrote:
>>> On Fri, 2018-10-19 at 19:46 +0200, Miklos Szeredi wrote:
>>>> How is it then that only STATX_ATIME is cleared and not the other
>>>> fields?
>>> 
>>> It isn't just the atime. We can also fail to revalidate the ctime
>>> and mtime if they are not being requested by the user.
>>> 
>>>> Note: junk != stale.  The statx definition doesn't talk about the
>>>> fields being up-to-date, except for AT_STATX_FORCE_SYNC, so stale
>>>> attributes are okay, and do not warrant clearing the result_mask.
>>> 
>>> I disagree. stale == junk here, because the default of
>>> AT_STATX_SYNC_AS_STAT is described by the manpage as "Do whatever
>>> stat(2) does." which this is not.
>> 
>> Ah, you are talking about this:
>> 
>>  /* Is the user requesting attributes that might need revalidation? */
>>  if (!(request_mask & (STATX_MODE|STATX_NLINK|STATX_ATIME|STATX_CTIME|
>>                    STATX_MTIME|STATX_UID|STATX_GID|
>>                    STATX_SIZE|STATX_BLOCKS)))
>>        goto out_no_update;
>> 
>> Well, if this is triggered for statx(...,  STATX_ATIME,
>> AT_STATX_SYNC_AS_STAT) and MNT_NOATIME, then yes, result will be
>> junk. Which means that the code is wrong, it shouldn't do that.
> 
> The problem is that vfs_getattr_nosec() populates stat->result_mask
> with a default of STATX_BASIC_STATS, which makes no sense unless you
> assume that the user will always ask for a superset of
> STATX_BASIC_STATS (or you assume that those attributes never need
> revalidation, which is obviously braindead).

I guess the assumption in the VFS code is that statx is mostly called
by local filesystems, for which STATX_BASIC_STATS is usually right,
so the basic VFS helper is OK to set those stats.  It should also be
possible for the filesystem to clear flags out of result_mask for
attributes that it doesn't want to return.

For filesystems that know what they are doing, it might just be best
to always clear stat->result_mask and fill in what they want, based
on the available attributes and request_mask rather than assuming
something is set by the caller.

Cheers, Andreas

>> Otherwise (if something other than STATX_ATIME or STATX_INO or
>> STATX_TYPE is given as well) it *will* do the same thing as what
>> stat(2) does, so in that case STATX_ATIME should not  be cleared (yet
>> it is cleared).
> 
> As far as I'm concerned, we can definitely get rid of the
> 
>        /*
>         * We may force a getattr if the user cares about atime.
>         *
>         * Note that we only have to check the vfsmount flags here:
>         *  - NFS always sets S_NOATIME by so checking it would give a
>         *    bogus result
>         *  - NFS never sets SB_NOATIME or SB_NODIRATIME so there is
>         *    no point in checking those.
>         */
>        if ((path->mnt->mnt_flags & MNT_NOATIME) ||
>            ((path->mnt->mnt_flags & MNT_NODIRATIME) && S_ISDIR(inode->i_mode)))
>                request_mask &= ~STATX_ATIME;
> 
> 
> however the rest needs to stay, or there is no way we can use statx()
> to allow optimised retrieval of only those attributes that your
> application cares about.


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

^ permalink raw reply

* Re: RFC: userspace exception fixups
From: Sean Christopherson @ 2018-11-06 15:37 UTC (permalink / raw)
  To: Andy Lutomirski, Jann Horn
  Cc: Dave Hansen, Linus Torvalds, Rich Felker, Dave Hansen,
	Jethro Beekman, Jarkko Sakkinen, Florian Weimer, Linux API,
	X86 ML, linux-arch, LKML, Peter Zijlstra, nhorman, npmccallum,
	Ayoun, Serge, shay.katz-zamir, linux-sgx, Andy Shevchenko,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, Carlos
In-Reply-To: <CALCETrXq1Kj=McxnPVjkscNq-b_7LeoFOdu7qKjHdvwcfFh9Og@mail.gmail.com>

On Fri, 2018-11-02 at 16:32 -0700, Andy Lutomirski wrote:
> On Fri, Nov 2, 2018 at 4:28 PM Jann Horn <jannh@google.com> wrote:
> > 
> > 
> > On Fri, Nov 2, 2018 at 11:04 PM Sean Christopherson
> > <sean.j.christopherson@intel.com> wrote:
> > > 
> > > On Fri, Nov 02, 2018 at 08:02:23PM +0100, Jann Horn wrote:
> > > > 
> > > > On Fri, Nov 2, 2018 at 7:27 PM Sean Christopherson
> > > > <sean.j.christopherson@intel.com> wrote:
> > > > > 
> > > > > On Fri, Nov 02, 2018 at 10:48:38AM -0700, Andy Lutomirski wrote:
> > > > > > 
> > > > > > This whole mechanism seems very complicated, and it's not clear
> > > > > > exactly what behavior user code wants.
> > > > > No argument there.  That's why I like the approach of dumping the
> > > > > exception to userspace without trying to do anything intelligent in
> > > > > the kernel.  Userspace can then do whatever it wants AND we don't
> > > > > have to worry about mucking with stacks.
> > > > > 
> > > > > One of the hiccups with the VDSO approach is that the enclave may
> > > > > want to use the untrusted stack, i.e. the stack that has the VDSO's
> > > > > stack frame.  For example, Intel's SDK uses the untrusted stack to
> > > > > pass parameters for EEXIT, which means an AEX might occur with what
> > > > > is effectively a bad stack from the VDSO's perspective.
> > > > What exactly does "uses the untrusted stack to pass parameters for
> > > > EEXIT" mean? I guess you're saying that the enclave is writing to
> > > > RSP+[0...some_positive_offset], and the written data needs to be
> > > > visible to the code outside the enclave afterwards?
> > > As is, they actually do it the other way around, i.e. negative offsets
> > > relative to the untrusted %RSP.  Going into the enclave there is no
> > > reserved space on the stack.  The SDK uses EEXIT like a function call,
> > > i.e. pushing parameters on the stack and making an call outside of the
> > > enclave, hence the name out-call.  This allows the SDK to handle any
> > > reasonable out-call without a priori knowledge of the application's
> > > maximum out-call "size".
> > But presumably this is bounded to be at most 128 bytes (the red zone
> > size), right? Otherwise this would be incompatible with
> > non-sigaltstack signal delivery.
> 
> I think Sean is saying that the enclave also updates RSP.

Yeah, the enclave saves/restores RSP from/to the current save state area.

> One might reasonably wonder how the SDX knows the offset from RSP to
> the function ID.  Presumably using RBP?

Here's pseudocode for how the SDK uses the untrusted stack, minus a
bunch of error checking and gory details.

The function ID and a pointer to a marshalling struct are passed to
the untrusted runtime via normal register params, e.g. RDI and RSI.
The marshalling struct is what's actually allocated on the untrusted
stack, like alloca() but more complex and explicit.  The marshalling
struct size is not artificially restricted by the SDK, e.g. AFAIK it
could span multiple 4k pages.


int sgx_out_call(const unsigned int func_index, void *marshalling_struct)
{
	struct sgx_encl_tls *tls = get_encl_tls();

	%RBP = tls->save_state_area[SSA_RBP];
	%RSP = tls->save_state_area[SSA_RSP];
	%RDI = func_index;
	%RSI = marshalling_struct;

	EEXIT

	/* magic elsewhere to get back here on an EENTER(OUT_CALL_RETURN) */
	return %RAX
}

void *sgx_alloc_untrusted_stack(size_t size)
{
	struct sgx_encl_tls *tls = get_encl_tls();
	struct sgx_out_call_context *context;
	void *tmp;

	/* create a frame on the trusted stack to hold the out-call context */
	tls->trusted_stack -= sizeof(struct sgx_out_call_context);

	/* save the untrusted %RSP into the out-call context */
	context = (struct sgx_out_call_context *)tls->trusted_stack;
	context->untrusted_stack = tls->save_state_area[SSA_RSP];

	/* allocate space on the untrusted stack */
	tmp = (void *)(tls->save_state_area[SSA_RSP] - size);
	tls->save_state_area[SSA_RSP] = tmp;

	return tmp;
}

void sgx_pop_untrusted_stack(void)
{
	struct sgx_encl_tls *tls = get_encl_tls();
	struct sgx_out_call_context *context;

	/* retrieve the current out-call context from the trusted stack */
	context = (struct sgx_out_call_context *)tls->trusted_stack;

	/* restore untrusted %RSP */
	tls->save_state_area[SSA_RSP] = context->untrusted_stack;

	/* pop the out-call context frame */
	tls->trusted_stack += sizeof(struct sgx_out_call_context);
}

int sgx_main(void)
{
	struct my_out_call_struct *params;

	params = sgx_alloc_untrusted_stack(sizeof(*params));

	params->0..N = XYZ;

	ret = sgx_out_call(DO_WORK, params);

	sgx_pop_untrusted_stack();

	return ret;
}

^ permalink raw reply

* Re: RFC: userspace exception fixups
From: Andy Lutomirski @ 2018-11-06 16:57 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Andy Lutomirski, Jann Horn, Dave Hansen, Linus Torvalds,
	Rich Felker, Dave Hansen, Jethro Beekman, Jarkko Sakkinen,
	Florian Weimer, Linux API, X86 ML, linux-arch, LKML,
	Peter Zijlstra, nhorman, npmccallum, Ayoun, Serge,
	shay.katz-zamir, linux-sgx, Andy Shevchenko, Thomas Gleixner,
	Ingo
In-Reply-To: <1541518670.7839.31.camel@intel.com>



> On Nov 6, 2018, at 7:37 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
>> On Fri, 2018-11-02 at 16:32 -0700, Andy Lutomirski wrote:
>>> On Fri, Nov 2, 2018 at 4:28 PM Jann Horn <jannh@google.com> wrote:
>>> 
>>> 
>>> On Fri, Nov 2, 2018 at 11:04 PM Sean Christopherson
>>> <sean.j.christopherson@intel.com> wrote:
>>>> 
>>>>> On Fri, Nov 02, 2018 at 08:02:23PM +0100, Jann Horn wrote:
>>>>> 
>>>>> On Fri, Nov 2, 2018 at 7:27 PM Sean Christopherson
>>>>> <sean.j.christopherson@intel.com> wrote:
>>>>>> 
>>>>>>> On Fri, Nov 02, 2018 at 10:48:38AM -0700, Andy Lutomirski wrote:
>>>>>>> 
>>>>>>> This whole mechanism seems very complicated, and it's not clear
>>>>>>> exactly what behavior user code wants.
>>>>>> No argument there.  That's why I like the approach of dumping the
>>>>>> exception to userspace without trying to do anything intelligent in
>>>>>> the kernel.  Userspace can then do whatever it wants AND we don't
>>>>>> have to worry about mucking with stacks.
>>>>>> 
>>>>>> One of the hiccups with the VDSO approach is that the enclave may
>>>>>> want to use the untrusted stack, i.e. the stack that has the VDSO's
>>>>>> stack frame.  For example, Intel's SDK uses the untrusted stack to
>>>>>> pass parameters for EEXIT, which means an AEX might occur with what
>>>>>> is effectively a bad stack from the VDSO's perspective.
>>>>> What exactly does "uses the untrusted stack to pass parameters for
>>>>> EEXIT" mean? I guess you're saying that the enclave is writing to
>>>>> RSP+[0...some_positive_offset], and the written data needs to be
>>>>> visible to the code outside the enclave afterwards?
>>>> As is, they actually do it the other way around, i.e. negative offsets
>>>> relative to the untrusted %RSP.  Going into the enclave there is no
>>>> reserved space on the stack.  The SDK uses EEXIT like a function call,
>>>> i.e. pushing parameters on the stack and making an call outside of the
>>>> enclave, hence the name out-call.  This allows the SDK to handle any
>>>> reasonable out-call without a priori knowledge of the application's
>>>> maximum out-call "size".
>>> But presumably this is bounded to be at most 128 bytes (the red zone
>>> size), right? Otherwise this would be incompatible with
>>> non-sigaltstack signal delivery.
>> 
>> I think Sean is saying that the enclave also updates RSP.
> 
> Yeah, the enclave saves/restores RSP from/to the current save state area.
> 
>> One might reasonably wonder how the SDX knows the offset from RSP to
>> the function ID.  Presumably using RBP?
> 
> Here's pseudocode for how the SDK uses the untrusted stack, minus a
> bunch of error checking and gory details.
> 
> The function ID and a pointer to a marshalling struct are passed to
> the untrusted runtime via normal register params, e.g. RDI and RSI.
> The marshalling struct is what's actually allocated on the untrusted
> stack, like alloca() but more complex and explicit.  The marshalling
> struct size is not artificially restricted by the SDK, e.g. AFAIK it
> could span multiple 4k pages.
> 
> 
> int sgx_out_call(const unsigned int func_index, void *marshalling_struct)
> {
>    struct sgx_encl_tls *tls = get_encl_tls();
> 
>    %RBP = tls->save_state_area[SSA_RBP];
>    %RSP = tls->save_state_area[SSA_RSP];
>    %RDI = func_index;
>    %RSI = marshalling_struct;
> 
>    EEXIT
> 
>    /* magic elsewhere to get back here on an EENTER(OUT_CALL_RETURN) */
>    return %RAX
> }
> 
> void *sgx_alloc_untrusted_stack(size_t size)
> {
>    struct sgx_encl_tls *tls = get_encl_tls();
>    struct sgx_out_call_context *context;
>    void *tmp;
> 
>    /* create a frame on the trusted stack to hold the out-call context */
>    tls->trusted_stack -= sizeof(struct sgx_out_call_context);
> 
>    /* save the untrusted %RSP into the out-call context */
>    context = (struct sgx_out_call_context *)tls->trusted_stack;
>    context->untrusted_stack = tls->save_state_area[SSA_RSP];
> 
>    /* allocate space on the untrusted stack */
>    tmp = (void *)(tls->save_state_area[SSA_RSP] - size);
>    tls->save_state_area[SSA_RSP] = tmp;
> 
>    return tmp;
> }
> 
> void sgx_pop_untrusted_stack(void)
> {
>    struct sgx_encl_tls *tls = get_encl_tls();
>    struct sgx_out_call_context *context;
> 
>    /* retrieve the current out-call context from the trusted stack */
>    context = (struct sgx_out_call_context *)tls->trusted_stack;
> 
>    /* restore untrusted %RSP */
>    tls->save_state_area[SSA_RSP] = context->untrusted_stack;
> 
>    /* pop the out-call context frame */
>    tls->trusted_stack += sizeof(struct sgx_out_call_context);
> }
> 
> int sgx_main(void)
> {
>    struct my_out_call_struct *params;
> 
>    params = sgx_alloc_untrusted_stack(sizeof(*params));
> 
>    params->0..N = XYZ;
> 
>    ret = sgx_out_call(DO_WORK, params);
> 
>    sgx_pop_untrusted_stack();
> 
>    return ret;
> }

So I guess the non-enclave code basically can’t trust its stack pointer because of these shenanigans. And the AEP code has to live with the fact that its RSP is basically arbitrary and probably can’t even be unwound by a debugger?  And the EENTER code has to deal with the fact that its red zone can be blatantly violated by the enclave?

I’m assuming it’s way too late for the SGX SDK to be changed to use a normal RPC mechanism? I’m a bit disappointed that enclaves can even manipulate outside state like this. I assume Intel had some reason for making it possible, but still.

^ permalink raw reply

* Re: RFC: userspace exception fixups
From: Dave Hansen @ 2018-11-06 17:00 UTC (permalink / raw)
  To: Sean Christopherson, Andy Lutomirski, Jann Horn
  Cc: Linus Torvalds, Rich Felker, Dave Hansen, Jethro Beekman,
	Jarkko Sakkinen, Florian Weimer, Linux API, X86 ML, linux-arch,
	LKML, Peter Zijlstra, nhorman, npmccallum, Ayoun, Serge,
	shay.katz-zamir, linux-sgx, Andy Shevchenko, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, Carlos O'Donell, adhemerval
In-Reply-To: <1541518670.7839.31.camel@intel.com>

On 11/6/18 7:37 AM, Sean Christopherson wrote:
> 
> void *sgx_alloc_untrusted_stack(size_t size)
> {
> 	struct sgx_encl_tls *tls = get_encl_tls();
> 	struct sgx_out_call_context *context;
> 	void *tmp;
> 
> 	/* create a frame on the trusted stack to hold the out-call context */
> 	tls->trusted_stack -= sizeof(struct sgx_out_call_context);
> 
> 	/* save the untrusted %RSP into the out-call context */
> 	context = (struct sgx_out_call_context *)tls->trusted_stack;
> 	context->untrusted_stack = tls->save_state_area[SSA_RSP];
> 
> 	/* allocate space on the untrusted stack */
> 	tmp = (void *)(tls->save_state_area[SSA_RSP] - size);
> 	tls->save_state_area[SSA_RSP] = tmp;
> 
> 	return tmp;
> }

Why does it bother to go to all the trouble of mucking with the
untrusted stack?  It could *easily* just leave it alone and do out-calls
if it needs to allocate memory for parameter storage.  Heck, that could
theoretically even be _on_ the stack if the untrusted runtime was being
clever.

The only downside would be that the untrusted runtime would have to keep
track of the space a bit more explicitly so it could be cleaned up if
the enclave didn't do it.

^ permalink raw reply

* Re: RFC: userspace exception fixups
From: Dave Hansen @ 2018-11-06 17:03 UTC (permalink / raw)
  To: Andy Lutomirski, Sean Christopherson
  Cc: Andy Lutomirski, Jann Horn, Linus Torvalds, Rich Felker,
	Dave Hansen, Jethro Beekman, Jarkko Sakkinen, Florian Weimer,
	Linux API, X86 ML, linux-arch, LKML, Peter Zijlstra, nhorman,
	npmccallum, Ayoun, Serge, shay.katz-zamir, linux-sgx,
	Andy Shevchenko, Thomas Gleixner, Ingo Molnar, Borislav Petkov
In-Reply-To: <AF4A5C77-0A79-403F-A205-0F93B7CD6E26@amacapital.net>

On 11/6/18 8:57 AM, Andy Lutomirski wrote:
> I’m assuming it’s way too late for the SGX SDK to be changed to use a
> normal RPC mechanism? I’m a bit disappointed that enclaves can even
> manipulate outside state like this. I assume Intel had some reason
> for making it possible, but still.

Just because it's architecturally possible doesn't mean it has to be a
part of the ABI for running enclaves under Linux.

It's not too late to change the SDK.  Intel does not and can not depend
on any behavior of Linux until code gets merged.

^ permalink raw reply

* Re: RFC: userspace exception fixups
From: Sean Christopherson @ 2018-11-06 17:19 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Andy Lutomirski, Jann Horn, Dave Hansen, Linus Torvalds,
	Rich Felker, Dave Hansen, Jethro Beekman, Jarkko Sakkinen,
	Florian Weimer, Linux API, X86 ML, linux-arch, LKML,
	Peter Zijlstra, nhorman, npmccallum, Ayoun, Serge,
	shay.katz-zamir, linux-sgx, Andy Shevchenko, Thomas Gleixner,
	Ingo
In-Reply-To: <AF4A5C77-0A79-403F-A205-0F93B7CD6E26@amacapital.net>

On Tue, 2018-11-06 at 08:57 -0800, Andy Lutomirski wrote:
>
> So I guess the non-enclave code basically can’t trust its stack pointer
> because of these shenanigans. And the AEP code has to live with the fact
> that its RSP is basically arbitrary and probably can’t even be unwound
> by a debugger?

The SDK provides a Python GDB plugin to hook into the out-call flow and
do more stack shenanigans.  From what I can tell it's fudging the stack
to make it look like a normal stack frame so the debugger can do it's
thing.

> And the EENTER code has to deal with the fact that its red zone can be
> blatantly violated by the enclave?

That's my understanding of things.  So yeah, if it wasn't obvious before,
the trusted and untrusted parts of the SDK are very tightly coupled.

^ permalink raw reply

* Re: RFC: userspace exception fixups
From: Andy Lutomirski @ 2018-11-06 18:20 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Andy Lutomirski, Jann Horn, Dave Hansen, Linus Torvalds,
	Rich Felker, Dave Hansen, Jethro Beekman, Jarkko Sakkinen,
	Florian Weimer, Linux API, X86 ML, linux-arch, LKML,
	Peter Zijlstra, nhorman, npmccallum, Ayoun, Serge,
	shay.katz-zamir, linux-sgx, Andy Shevchenko, Thomas Gleixner,
	Ingo
In-Reply-To: <1541524750.7839.51.camel@intel.com>




> On Nov 6, 2018, at 9:19 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
>> On Tue, 2018-11-06 at 08:57 -0800, Andy Lutomirski wrote:
>> 
>> So I guess the non-enclave code basically can’t trust its stack pointer
>> because of these shenanigans. And the AEP code has to live with the fact
>> that its RSP is basically arbitrary and probably can’t even be unwound
>> by a debugger?
> 
> The SDK provides a Python GDB plugin to hook into the out-call flow and
> do more stack shenanigans.  From what I can tell it's fudging the stack
> to make it look like a normal stack frame so the debugger can do it's
> thing.
> 
>> And the EENTER code has to deal with the fact that its red zone can be
>> blatantly violated by the enclave?
> 
> That's my understanding of things.  So yeah, if it wasn't obvious before,
> the trusted and untrusted parts of the SDK are very tightly coupled.

Yuck. Just how far does this right coupling go?  If there are enclaves that play with, say, FSBASE or GSBASE, we’re going to start having problems. And the SGX handling of PKRU is complicated at best.

I almost feel like the right solution is to call into SGX on its own private stack or maybe even its own private address space. 

^ permalink raw reply

* Re: RFC: userspace exception fixups
From: Dave Hansen @ 2018-11-06 18:41 UTC (permalink / raw)
  To: Andy Lutomirski, Sean Christopherson
  Cc: Andy Lutomirski, Jann Horn, Linus Torvalds, Rich Felker,
	Dave Hansen, Jethro Beekman, Jarkko Sakkinen, Florian Weimer,
	Linux API, X86 ML, linux-arch, LKML, Peter Zijlstra, nhorman,
	npmccallum, Ayoun, Serge, shay.katz-zamir, linux-sgx,
	Andy Shevchenko, Thomas Gleixner, Ingo Molnar, Borislav Petkov
In-Reply-To: <22596E35-F5D1-4935-86AB-B510DCA0FABE@amacapital.net>

On 11/6/18 10:20 AM, Andy Lutomirski wrote:
> I almost feel like the right solution is to call into SGX on its own
> private stack or maybe even its own private address space.

Yeah, I had the same gut feeling.  Couldn't the debugger even treat the
enclave like its own "thread" with its own stack and its own set of
registers and context?  That seems like a much more workable model than
trying to weave it together with the EENTER context.

^ permalink raw reply

* Re: [PATCH v5 21/27] x86/cet/shstk: Introduce WRUSS instruction
From: Dave Hansen @ 2018-11-06 18:43 UTC (permalink / raw)
  To: Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
	linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
	Arnd Bergmann, Andy Lutomirski, Balbir Singh, Cyrill Gorcunov,
	Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
	Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit
In-Reply-To: <20181011151523.27101-22-yu-cheng.yu@intel.com>

On 10/11/18 8:15 AM, Yu-cheng Yu wrote:
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -1305,6 +1305,15 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
>  		error_code |= X86_PF_USER;
>  		flags |= FAULT_FLAG_USER;
>  	} else {
> +		/*
> +		 * WRUSS is a kernel instruction and but writes
> +		 * to user shadow stack.  When a fault occurs,
> +		 * both X86_PF_USER and X86_PF_SHSTK are set.
> +		 * Clear X86_PF_USER here.
> +		 */
> +		if ((error_code & (X86_PF_USER | X86_PF_SHSTK)) ==
> +		    (X86_PF_USER | X86_PF_SHSTK))
> +			error_code &= ~X86_PF_USER;
This hunk of code basically points out that the architecture of WRUSS is
broken for Linux.  The setting of X86_PF_USER for a ring-0 instruction
really is a mis-feature of the architecture for us and we *undo* it in
software which is unfortunate.  Wish I would have caught this earlier.

Andy, note that this is another case where hw_error_code and
sw_error_code will diverge, unfortunately.

Anyway, this is going to necessitate some comment updates in the page
fault code.  Yu-cheng, you are going to collide with some recent changes
I made to the page fault code.  Please be careful with the context when
you do the merge and make sure that all the new comments stay correct.

^ permalink raw reply

* Re: [PATCH v5 21/27] x86/cet/shstk: Introduce WRUSS instruction
From: Andy Lutomirski @ 2018-11-06 18:55 UTC (permalink / raw)
  To: Dave Hansen
  Cc: Yu-cheng Yu, X86 ML, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
	LKML, open list:DOCUMENTATION, Linux-MM, linux-arch, Linux API,
	Arnd Bergmann, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
	Eugene Syromiatnikov, Florian Weimer, H. J. Lu, Jann Horn,
	Jonathan Corbet, Kees Cook, Mike Kravetz <mike.krav>
In-Reply-To: <ee5a93f7-ed42-dcc5-0e55-e73ac2637e84@intel.com>

On Tue, Nov 6, 2018 at 10:43 AM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 10/11/18 8:15 AM, Yu-cheng Yu wrote:
> > --- a/arch/x86/mm/fault.c
> > +++ b/arch/x86/mm/fault.c
> > @@ -1305,6 +1305,15 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
> >               error_code |= X86_PF_USER;
> >               flags |= FAULT_FLAG_USER;
> >       } else {
> > +             /*
> > +              * WRUSS is a kernel instruction and but writes
> > +              * to user shadow stack.  When a fault occurs,
> > +              * both X86_PF_USER and X86_PF_SHSTK are set.
> > +              * Clear X86_PF_USER here.
> > +              */
> > +             if ((error_code & (X86_PF_USER | X86_PF_SHSTK)) ==
> > +                 (X86_PF_USER | X86_PF_SHSTK))
> > +                     error_code &= ~X86_PF_USER;
> This hunk of code basically points out that the architecture of WRUSS is
> broken for Linux.  The setting of X86_PF_USER for a ring-0 instruction
> really is a mis-feature of the architecture for us and we *undo* it in
> software which is unfortunate.  Wish I would have caught this earlier.
>
> Andy, note that this is another case where hw_error_code and
> sw_error_code will diverge, unfortunately.
>
> Anyway, this is going to necessitate some comment updates in the page
> fault code.  Yu-cheng, you are going to collide with some recent changes
> I made to the page fault code.  Please be careful with the context when
> you do the merge and make sure that all the new comments stay correct.

I'm going to send a patch set in the next day or two that cleans it up
further and is probably good preparation for WRUSS.

^ permalink raw reply

* Re: RFC: userspace exception fixups
From: Andy Lutomirski @ 2018-11-06 19:02 UTC (permalink / raw)
  To: Dave Hansen
  Cc: Christopherson, Sean J, Andrew Lutomirski, Jann Horn,
	Linus Torvalds, Rich Felker, Dave Hansen, Jethro Beekman,
	Jarkko Sakkinen, Florian Weimer, Linux API, X86 ML, linux-arch,
	LKML, Peter Zijlstra, nhorman, npmccallum, Ayoun, Serge,
	shay.katz-zamir, linux-sgx, Andy Shevchenko,
	Thomas Gleixner <tglx@
In-Reply-To: <c9659222-efc6-b73a-ce48-30be8bdc5397@intel.com>

On Tue, Nov 6, 2018 at 10:41 AM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 11/6/18 10:20 AM, Andy Lutomirski wrote:
> > I almost feel like the right solution is to call into SGX on its own
> > private stack or maybe even its own private address space.
>
> Yeah, I had the same gut feeling.  Couldn't the debugger even treat the
> enclave like its own "thread" with its own stack and its own set of
> registers and context?  That seems like a much more workable model than
> trying to weave it together with the EENTER context.

So maybe the API should be, roughly

sgx_exit_reason_t sgx_enter_enclave(pointer_to_enclave, struct
host_state *state);
sgx_exit_reason_t sgx_resume_enclave(same args);

where host_state is something like:

struct host_state {
  unsigned long bp, sp, ax, bx, cx, dx, si, di;
};

and the values in host_state explicitly have nothing to do with the
actual host registers.  So, if you want to use the outcall mechanism,
you'd allocate some memory, point sp to that memory, call
sgx_enter_enclave(), and then read that memory to do the outcall.

Actually implementing this would be distinctly nontrivial, and would
almost certainly need some degree of kernel help to avoid an explosion
when a signal gets delivered while we have host_state.sp loaded into
the actual SP register.  Maybe rseq could help with this?

The ISA here is IMO not well thought through.

^ permalink raw reply

* Re: RFC: userspace exception fixups
From: Dave Hansen @ 2018-11-06 19:22 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Christopherson, Sean J, Jann Horn, Linus Torvalds, Rich Felker,
	Dave Hansen, Jethro Beekman, Jarkko Sakkinen, Florian Weimer,
	Linux API, X86 ML, linux-arch, LKML, Peter Zijlstra, nhorman,
	npmccallum, Ayoun, Serge, shay.katz-zamir, linux-sgx,
	Andy Shevchenko, Thomas Gleixner, Ingo Molnar
In-Reply-To: <CALCETrWBV=1JbAKYn2Jy2LxkGZQvKRtFRnrWUMoejrwQe73VHw@mail.gmail.com>

On 11/6/18 11:02 AM, Andy Lutomirski wrote:
> On Tue, Nov 6, 2018 at 10:41 AM Dave Hansen <dave.hansen@intel.com> wrote:
>>
>> On 11/6/18 10:20 AM, Andy Lutomirski wrote:
>>> I almost feel like the right solution is to call into SGX on its own
>>> private stack or maybe even its own private address space.
>>
>> Yeah, I had the same gut feeling.  Couldn't the debugger even treat the
>> enclave like its own "thread" with its own stack and its own set of
>> registers and context?  That seems like a much more workable model than
>> trying to weave it together with the EENTER context.
> 
> So maybe the API should be, roughly
> 
> sgx_exit_reason_t sgx_enter_enclave(pointer_to_enclave, struct
> host_state *state);
> sgx_exit_reason_t sgx_resume_enclave(same args);
> 
> where host_state is something like:
> 
> struct host_state {
>   unsigned long bp, sp, ax, bx, cx, dx, si, di;
> };
> 
> and the values in host_state explicitly have nothing to do with the
> actual host registers.  So, if you want to use the outcall mechanism,
> you'd allocate some memory, point sp to that memory, call
> sgx_enter_enclave(), and then read that memory to do the outcall.

Ah, so instead of the enclave rudely "hijacking" the EENTER context, we
have it nicely return and nicely _hint_ to the calling context what it
would like to do.  Then, the EENTER context can make a controlled
transition over to the requested context.

> Actually implementing this would be distinctly nontrivial, and would
> almost certainly need some degree of kernel help to avoid an explosion
> when a signal gets delivered while we have host_state.sp loaded into
> the actual SP register.  Maybe rseq could help with this?

As long as the memory pointed to by host_state.sp is valid and can hold
the signal frame (grows down without clobbering anything), what goes
boom?  The signal handling would push a signal frame and call the
handler.  It would have a shallow-looking stack, but the handler could
just do its normal business and return from the signal where the frame
would get popped and continue with %rsp=host_state.sp, blissfully
unaware of the signal ever having happened.

^ permalink raw reply

* Re: [PATCH v6 1/1] ns: add binfmt_misc to the user namespace
From: Jann Horn @ 2018-11-06 19:40 UTC (permalink / raw)
  To: Laurent Vivier, Andrew Morton
  Cc: Eric W. Biederman, Andrei Vagin, kernel list, James Bottomley,
	Linux API, linux-fsdevel, Al Viro, containers, dima
In-Reply-To: <4a1ec0cf-dce7-4193-6946-d46d63398c2d@vivier.eu>

On Tue, Oct 30, 2018 at 9:51 AM Laurent Vivier <laurent@vivier.eu> wrote:
> Le 24/10/2018 à 19:15, Laurent Vivier a écrit :
> > On 16/10/2018 17:22, Andrei Vagin wrote:
> >> On Wed, Oct 10, 2018 at 06:14:30PM +0200, Laurent Vivier wrote:
> >>> This patch allows to have a different binfmt_misc configuration
> >>> for each new user namespace. By default, the binfmt_misc configuration
> >>> is the one of the previous level, but if the binfmt_misc filesystem is
> >>> mounted in the new namespace a new empty binfmt instance is created and
> >>> used in this namespace.
> >>>
> >>> For instance, using "unshare" we can start a chroot of another
> >>> architecture and configure the binfmt_misc interpreter without being root
> >>> to run the binaries in this chroot.
> >>>
> >>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> >>
> >> Acked-by: Andrei Vagin <avagin@gmail.com>
> >>
> >> Thanks,
> >> Andrei
> >>
> >
> > I don't konw who is the maintainer for this part,

I think Andrew Morton is the right maintainer here.

> > but is there any
> > chance to have this merged in 4.20?
> >
>
> I'd really want to have this merged.
>
> I have some real use cases for this:
>
> 1- to allow a non root user to run a container (with "unshare" for
> instance) with its own binfmt_misc configuration. For instance, like we
> provide a disk image and ask an ordinary user to run it with his
> favorite VM hypervisor, we can provide a tar.gz containing our own
> interpreter and just ask him to unshare+chroot to the exploded file tree,
>
> 2- to allow to run automatic tests of an interpreter on a machine
> without having to change the global configuration of the system. I have
> in mind to add some tests in Avocado to automatically test
> qemu-linux-user in containers, so the interpreter path can depend on the
> build path and possibly run them concurrently,
>
> 3- to select an interpreter by container. For instance, on the
> qemu-devel mailing list, we have a waiting patch to add the bFLT
> interpreter binfmt_misc configuration, but the bFLT doesn't provide the
> CPU type in magic/mask. So it would be interesting to be able to select
> also a bFLT interpreter by container, as we know the CPU architecture we
> have in each chroot/container,
>
> 4- another example to select an interpreter by container is qemu-mips
> and qemu-misn32 share the same magic/mask because only the kernel API
> changes, so we can't configure both on the system (but I agree it's a
> QEMU bug: they should be merged and the kernel API be selected at runtime).

^ permalink raw reply

* Re: RFC: userspace exception fixups
From: Andy Lutomirski @ 2018-11-06 20:12 UTC (permalink / raw)
  To: Dave Hansen
  Cc: Andy Lutomirski, Christopherson, Sean J, Jann Horn,
	Linus Torvalds, Rich Felker, Dave Hansen, Jethro Beekman,
	Jarkko Sakkinen, Florian Weimer, Linux API, X86 ML, linux-arch,
	LKML, Peter Zijlstra, nhorman, npmccallum, Ayoun, Serge,
	shay.katz-zamir, linux-sgx, Andy Shevchenko, Thomas Gleixner
In-Reply-To: <b9c53669-cd27-e3bc-3d62-f47c77029c43@intel.com>



> On Nov 6, 2018, at 11:22 AM, Dave Hansen <dave.hansen@intel.com> wrote:
> 
>> On 11/6/18 11:02 AM, Andy Lutomirski wrote:
>>> On Tue, Nov 6, 2018 at 10:41 AM Dave Hansen <dave.hansen@intel.com> wrote:
>>> 
>>>> On 11/6/18 10:20 AM, Andy Lutomirski wrote:
>>>> I almost feel like the right solution is to call into SGX on its own
>>>> private stack or maybe even its own private address space.
>>> 
>>> Yeah, I had the same gut feeling.  Couldn't the debugger even treat the
>>> enclave like its own "thread" with its own stack and its own set of
>>> registers and context?  That seems like a much more workable model than
>>> trying to weave it together with the EENTER context.
>> 
>> So maybe the API should be, roughly
>> 
>> sgx_exit_reason_t sgx_enter_enclave(pointer_to_enclave, struct
>> host_state *state);
>> sgx_exit_reason_t sgx_resume_enclave(same args);
>> 
>> where host_state is something like:
>> 
>> struct host_state {
>>  unsigned long bp, sp, ax, bx, cx, dx, si, di;
>> };
>> 
>> and the values in host_state explicitly have nothing to do with the
>> actual host registers.  So, if you want to use the outcall mechanism,
>> you'd allocate some memory, point sp to that memory, call
>> sgx_enter_enclave(), and then read that memory to do the outcall.
> 
> Ah, so instead of the enclave rudely "hijacking" the EENTER context, we
> have it nicely return and nicely _hint_ to the calling context what it
> would like to do.  Then, the EENTER context can make a controlled
> transition over to the requested context.

Exactly. And existing enclaves keep working — their rudeness is just magically translated into a hint!

> 
>> Actually implementing this would be distinctly nontrivial, and would
>> almost certainly need some degree of kernel help to avoid an explosion
>> when a signal gets delivered while we have host_state.sp loaded into
>> the actual SP register.  Maybe rseq could help with this?
> 
> As long as the memory pointed to by host_state.sp is valid and can hold
> the signal frame (grows down without clobbering anything), what goes
> boom?  The signal handling would push a signal frame and call the
> handler.  It would have a shallow-looking stack, but the handler could
> just do its normal business and return from the signal where the frame
> would get popped and continue with %rsp=host_state.sp, blissfully
> unaware of the signal ever having happened.

True, but what if we have a nasty enclave that writes to memory just below SP *before* decrementing SP?

I suspect that rseq really can be used for this with only minimal-ish modifications.  Or we could stick this in the vDSO with some appropriate fixups in the kernel.

^ permalink raw reply

* Re: [PATCH v5 21/27] x86/cet/shstk: Introduce WRUSS instruction
From: Yu-cheng Yu @ 2018-11-06 20:21 UTC (permalink / raw)
  To: Dave Hansen, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
	linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
	Arnd Bergmann, Andy Lutomirski, Balbir Singh, Cyrill Gorcunov,
	Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
	Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit
In-Reply-To: <ee5a93f7-ed42-dcc5-0e55-e73ac2637e84@intel.com>

On Tue, 2018-11-06 at 10:43 -0800, Dave Hansen wrote:
> On 10/11/18 8:15 AM, Yu-cheng Yu wrote:
> > --- a/arch/x86/mm/fault.c
> > +++ b/arch/x86/mm/fault.c
> > @@ -1305,6 +1305,15 @@ __do_page_fault(struct pt_regs *regs, unsigned long
> > error_code,
> >  		error_code |= X86_PF_USER;
> >  		flags |= FAULT_FLAG_USER;
> >  	} else {
> > +		/*
> > +		 * WRUSS is a kernel instruction and but writes
> > +		 * to user shadow stack.  When a fault occurs,
> > +		 * both X86_PF_USER and X86_PF_SHSTK are set.
> > +		 * Clear X86_PF_USER here.
> > +		 */
> > +		if ((error_code & (X86_PF_USER | X86_PF_SHSTK)) ==
> > +		    (X86_PF_USER | X86_PF_SHSTK))
> > +			error_code &= ~X86_PF_USER;
> 
> This hunk of code basically points out that the architecture of WRUSS is
> broken for Linux.  The setting of X86_PF_USER for a ring-0 instruction
> really is a mis-feature of the architecture for us and we *undo* it in
> software which is unfortunate.  Wish I would have caught this earlier.
> 
> Andy, note that this is another case where hw_error_code and
> sw_error_code will diverge, unfortunately.
> 
> Anyway, this is going to necessitate some comment updates in the page
> fault code.  Yu-cheng, you are going to collide with some recent changes
> I made to the page fault code.  Please be careful with the context when
> you do the merge and make sure that all the new comments stay correct.

Ok.  Thanks!

Yu-cheng

^ permalink raw reply

* Re: RFC: userspace exception fixups
From: Dave Hansen @ 2018-11-06 21:00 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Andy Lutomirski, Christopherson, Sean J, Jann Horn,
	Linus Torvalds, Rich Felker, Dave Hansen, Jethro Beekman,
	Jarkko Sakkinen, Florian Weimer, Linux API, X86 ML, linux-arch,
	LKML, Peter Zijlstra, nhorman, npmccallum, Ayoun, Serge,
	shay.katz-zamir, linux-sgx, Andy Shevchenko, Thomas Gleixner
In-Reply-To: <1C426267-492F-4AE7-8BE8-C7FE278531F9@amacapital.net>

On 11/6/18 12:12 PM, Andy Lutomirski wrote:
> True, but what if we have a nasty enclave that writes to memory just
> below SP *before* decrementing SP?

Yeah, that would be unfortunate.  If an enclave did this (roughly):

	1. EENTER
	2. Hardware sets eenter_hwframe->sp = %sp
	3. Enclave runs... wants to do out-call
	4. Enclave sets up parameters:
		memcpy(&eenter_hwframe->sp[-offset], arg1, size);
		...
	5. Enclave sets eenter_hwframe->sp -= offset

If we got a signal between 4 and 5, we'd clobber the copy of 'arg1' that
was on the stack.  The enclave could easily fix this by moving ->sp first.

But, this is one of those "fun" parts of the ABI that I think we need to
talk about.  If we do this, we also basically require that the code
which handles asynchronous exits must *not* write to the stack.  That's
not hard because it's typically just a single ERESUME instruction, but
it *is* a requirement.

It means fun stuff like that you absolutely can't just async-exit to C code.

^ permalink raw reply

* Re: RFC: userspace exception fixups
From: Andy Lutomirski @ 2018-11-06 21:07 UTC (permalink / raw)
  To: Dave Hansen
  Cc: Andy Lutomirski, Christopherson, Sean J, Jann Horn,
	Linus Torvalds, Rich Felker, Dave Hansen, Jethro Beekman,
	Jarkko Sakkinen, Florian Weimer, Linux API, X86 ML, linux-arch,
	LKML, Peter Zijlstra, nhorman, npmccallum, Ayoun, Serge,
	shay.katz-zamir, linux-sgx, Andy Shevchenko, Thomas Gleixner
In-Reply-To: <209cf4a5-eda9-2495-539f-fed22252cf02@intel.com>



> On Nov 6, 2018, at 1:00 PM, Dave Hansen <dave.hansen@intel.com> wrote:
> 
>> On 11/6/18 12:12 PM, Andy Lutomirski wrote:
>> True, but what if we have a nasty enclave that writes to memory just
>> below SP *before* decrementing SP?
> 
> Yeah, that would be unfortunate.  If an enclave did this (roughly):
> 
>    1. EENTER
>    2. Hardware sets eenter_hwframe->sp = %sp
>    3. Enclave runs... wants to do out-call
>    4. Enclave sets up parameters:
>        memcpy(&eenter_hwframe->sp[-offset], arg1, size);
>        ...
>    5. Enclave sets eenter_hwframe->sp -= offset
> 
> If we got a signal between 4 and 5, we'd clobber the copy of 'arg1' that
> was on the stack.  The enclave could easily fix this by moving ->sp first.
> 
> But, this is one of those "fun" parts of the ABI that I think we need to
> talk about.  If we do this, we also basically require that the code
> which handles asynchronous exits must *not* write to the stack.  That's
> not hard because it's typically just a single ERESUME instruction, but
> it *is* a requirement.
> 

I was assuming that the async exit stuff was completely hidden by the API. The AEP code would decide whether the exit got fixed up by the kernel (which may or may not be easy to tell — can the code even tell without kernel help whether it was, say, an IRQ vs #UD?) and then either do ERESUME or cause sgx_enter_enclave() to return with an appropriate return value.

^ permalink raw reply

* Re: RFC: userspace exception fixups
From: Andy Lutomirski @ 2018-11-06 21:41 UTC (permalink / raw)
  To: Dave Hansen
  Cc: Andrew Lutomirski, Christopherson, Sean J, Jann Horn,
	Linus Torvalds, Rich Felker, Dave Hansen, Jethro Beekman,
	Jarkko Sakkinen, Florian Weimer, Linux API, X86 ML, linux-arch,
	LKML, Peter Zijlstra, nhorman, npmccallum, Ayoun, Serge,
	shay.katz-zamir, linux-sgx, Andy Shevchenko,
	Thomas Gleixner <tglx@
In-Reply-To: <9B76E95B-5745-412E-8007-7FAA7F83D6FB@amacapital.net>

On Tue, Nov 6, 2018 at 1:07 PM Andy Lutomirski <luto@amacapital.net> wrote:
>
>
>
> > On Nov 6, 2018, at 1:00 PM, Dave Hansen <dave.hansen@intel.com> wrote:
> >
> >> On 11/6/18 12:12 PM, Andy Lutomirski wrote:
> >> True, but what if we have a nasty enclave that writes to memory just
> >> below SP *before* decrementing SP?
> >
> > Yeah, that would be unfortunate.  If an enclave did this (roughly):
> >
> >    1. EENTER
> >    2. Hardware sets eenter_hwframe->sp = %sp
> >    3. Enclave runs... wants to do out-call
> >    4. Enclave sets up parameters:
> >        memcpy(&eenter_hwframe->sp[-offset], arg1, size);
> >        ...
> >    5. Enclave sets eenter_hwframe->sp -= offset
> >
> > If we got a signal between 4 and 5, we'd clobber the copy of 'arg1' that
> > was on the stack.  The enclave could easily fix this by moving ->sp first.
> >
> > But, this is one of those "fun" parts of the ABI that I think we need to
> > talk about.  If we do this, we also basically require that the code
> > which handles asynchronous exits must *not* write to the stack.  That's
> > not hard because it's typically just a single ERESUME instruction, but
> > it *is* a requirement.
> >
>
> I was assuming that the async exit stuff was completely hidden by the API. The AEP code would decide whether the exit got fixed up by the kernel (which may or may not be easy to tell — can the code even tell without kernel help whether it was, say, an IRQ vs #UD?) and then either do ERESUME or cause sgx_enter_enclave() to return with an appropriate return value.
>
>

Sean, how does the current SDK AEX handler decide whether to do
EENTER, ERESUME, or just bail and consider the enclave dead?  It seems
like the *CPU* could give a big hint, but I don't see where there is
any architectural indication of why the AEX code got called or any
obvious way for the user code to know whether the exit was fixed up by
the kernel?

^ permalink raw reply

* Re: RFC: userspace exception fixups
From: Sean Christopherson @ 2018-11-06 21:59 UTC (permalink / raw)
  To: Andy Lutomirski, Dave Hansen
  Cc: Jann Horn, Linus Torvalds, Rich Felker, Dave Hansen,
	Jethro Beekman, Jarkko Sakkinen, Florian Weimer, Linux API,
	X86 ML, linux-arch, LKML, Peter Zijlstra, nhorman, npmccallum,
	Ayoun, Serge, shay.katz-zamir, linux-sgx, Andy Shevchenko,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov
In-Reply-To: <CALCETrV=iodOQhvXAyjs0TQNbCaFdkhrZqRHvWTnBfo2m0qXpA@mail.gmail.com>

On Tue, 2018-11-06 at 13:41 -0800, Andy Lutomirski wrote:
> On Tue, Nov 6, 2018 at 1:07 PM Andy Lutomirski <luto@amacapital.net> wrote:
> > 
> > > 
> > > On Nov 6, 2018, at 1:00 PM, Dave Hansen <dave.hansen@intel.com> wrote:
> > > 
> > > > 
> > > > On 11/6/18 12:12 PM, Andy Lutomirski wrote:
> > > > True, but what if we have a nasty enclave that writes to memory just
> > > > below SP *before* decrementing SP?
> > > Yeah, that would be unfortunate.  If an enclave did this (roughly):
> > > 
> > >    1. EENTER
> > >    2. Hardware sets eenter_hwframe->sp = %sp
> > >    3. Enclave runs... wants to do out-call
> > >    4. Enclave sets up parameters:
> > >        memcpy(&eenter_hwframe->sp[-offset], arg1, size);
> > >        ...
> > >    5. Enclave sets eenter_hwframe->sp -= offset
> > > 
> > > If we got a signal between 4 and 5, we'd clobber the copy of 'arg1' that
> > > was on the stack.  The enclave could easily fix this by moving ->sp first.
> > > 
> > > But, this is one of those "fun" parts of the ABI that I think we need to
> > > talk about.  If we do this, we also basically require that the code
> > > which handles asynchronous exits must *not* write to the stack.  That's
> > > not hard because it's typically just a single ERESUME instruction, but
> > > it *is* a requirement.
> > > 
> > I was assuming that the async exit stuff was completely hidden by the API. The AEP code would decide whether the exit got fixed up by the kernel (which may or may not be easy to tell — can the
> > code even tell without kernel help whether it was, say, an IRQ vs #UD?) and then either do ERESUME or cause sgx_enter_enclave() to return with an appropriate return value.
> > 
> > 
> Sean, how does the current SDK AEX handler decide whether to do
> EENTER, ERESUME, or just bail and consider the enclave dead?  It seems
> like the *CPU* could give a big hint, but I don't see where there is
> any architectural indication of why the AEX code got called or any
> obvious way for the user code to know whether the exit was fixed up by
> the kernel?

The SDK "unconditionally" does ERESUME at the AEP location, but that's
bit misleading because its signal handler may muck with the context's
RIP, e.g. to abort the enclave on a fatal fault.

On an event/exception from within an enclave, the event is immediately
delivered after loading synthetic state and changing RIP to the AEP.
In other words, jamming CPU state is essentially a bunch of vectoring
ucode preamble, but from software's perspective it's a normal event
that happens to point at the AEP instead of somewhere in the enclave.
And because the signals the SDK cares about are all synchronous, the
SDK can simply hardcode ERESUME at the AEP since all of the fault logic
resides in its signal handler.  IRQs and whatnot simply trampoline back
into the enclave.

Userspace can do something funky instead of ERESUME, but only *after*
IRET/RSM/VMRESUME has returned to the AEP location, and in Linux's
case, after the trap handler has run.

Jumping back a bit, how much do we care about preventing userspace
from doing stupid things?  I did a quick POC on the idea of hardcoding
fixup for the ENCLU opcode, and the basic idea checks out.  The code
is fairly minimal and doesn't impact the core functionality of the SDK.
They'd need to redo their trap handling to move it from the signal
handler to inline, but their stack shenanigans won't be any more broken
than they already are.

^ permalink raw reply

* Re: RFC: userspace exception fixups
From: Andy Lutomirski @ 2018-11-06 23:00 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Andy Lutomirski, Dave Hansen, Jann Horn, Linus Torvalds,
	Rich Felker, Dave Hansen, Jethro Beekman, Jarkko Sakkinen,
	Florian Weimer, Linux API, X86 ML, linux-arch, LKML,
	Peter Zijlstra, nhorman, npmccallum, Ayoun, Serge,
	shay.katz-zamir, linux-sgx, Andy Shevchenko, Thomas Gleixner
In-Reply-To: <1541541565.8854.13.camel@intel.com>



>> On Nov 6, 2018, at 1:59 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>> 
>>> On Tue, 2018-11-06 at 13:41 -0800, Andy Lutomirski wrote:
>>>> On Tue, Nov 6, 2018 at 1:07 PM Andy Lutomirski <luto@amacapital.net> wrote:
>>>> 
>>>> 
>>>>> On Nov 6, 2018, at 1:00 PM, Dave Hansen <dave.hansen@intel.com> wrote:
>>>>> 
>>>>> 
>>>>> On 11/6/18 12:12 PM, Andy Lutomirski wrote:
>>>>> True, but what if we have a nasty enclave that writes to memory just
>>>>> below SP *before* decrementing SP?
>>>> Yeah, that would be unfortunate.  If an enclave did this (roughly):
>>>> 
>>>>    1. EENTER
>>>>    2. Hardware sets eenter_hwframe->sp = %sp
>>>>    3. Enclave runs... wants to do out-call
>>>>    4. Enclave sets up parameters:
>>>>        memcpy(&eenter_hwframe->sp[-offset], arg1, size);
>>>>        ...
>>>>    5. Enclave sets eenter_hwframe->sp -= offset
>>>> 
>>>> If we got a signal between 4 and 5, we'd clobber the copy of 'arg1' that
>>>> was on the stack.  The enclave could easily fix this by moving ->sp first.
>>>> 
>>>> But, this is one of those "fun" parts of the ABI that I think we need to
>>>> talk about.  If we do this, we also basically require that the code
>>>> which handles asynchronous exits must *not* write to the stack.  That's
>>>> not hard because it's typically just a single ERESUME instruction, but
>>>> it *is* a requirement.
>>> I was assuming that the async exit stuff was completely hidden by the API. The AEP code would decide whether the exit got fixed up by the kernel (which may or may not be easy to tell — can the
>>> code even tell without kernel help whether it was, say, an IRQ vs #UD?) and then either do ERESUME or cause sgx_enter_enclave() to return with an appropriate return value.
>> Sean, how does the current SDK AEX handler decide whether to do
>> EENTER, ERESUME, or just bail and consider the enclave dead?  It seems
>> like the *CPU* could give a big hint, but I don't see where there is
>> any architectural indication of why the AEX code got called or any
>> obvious way for the user code to know whether the exit was fixed up by
>> the kernel?
> 
> The SDK "unconditionally" does ERESUME at the AEP location, but that's
> bit misleading because its signal handler may muck with the context's
> RIP, e.g. to abort the enclave on a fatal fault.
> 
> On an event/exception from within an enclave, the event is immediately
> delivered after loading synthetic state and changing RIP to the AEP.
> In other words, jamming CPU state is essentially a bunch of vectoring
> ucode preamble, but from software's perspective it's a normal event
> that happens to point at the AEP instead of somewhere in the enclave.
> And because the signals the SDK cares about are all synchronous, the
> SDK can simply hardcode ERESUME at the AEP since all of the fault logic
> resides in its signal handler.  IRQs and whatnot simply trampoline back
> into the enclave.
> 
> Userspace can do something funky instead of ERESUME, but only *after*
> IRET/RSM/VMRESUME has returned to the AEP location, and in Linux's
> case, after the trap handler has run.
> 
> Jumping back a bit, how much do we care about preventing userspace
> from doing stupid things? 

My general feeling is that userspace should be allowed to do apparently stupid things. For example, as far as the kernel is concerned, Wine and DOSEMU are just user programs that do stupid things. Linux generally tries to provide a reasonably complete view of architectural behavior. This is in contrast to, say, Windows, where IIUC doing an unapproved WRFSBASE May cause very odd behavior indeed. So magic fixups that do non-architectural things are not so great.

The flip side, of course, is that the architecture is arguably inherently erratic here, and it’s apparently impossible to have an SGX library with sane semantics without some kernel assistance.

So if we can make my straw man API work, perhaps with vDSO or rseq-like help, then the official SDK can use it, but less well behaved programs can still mostly work.  (Modulo Linux’s non-support for EINITTOKEN, of course.)

Thinking about it some more, the major sticking point may be finding the RIP and stack frame of EENTER in the AEP code or in its fixup. The vDSO can’t use TLS without serious hackery.  We could massively abuse WRFSBASE, but that’s really ugly.

(How does the Windows case work?  If there’s an exception after the untrusted stack allocation and before EEXIT and SEH tries to handle it, how does the unwinder figure out where to start?)

>  I did a quick POC on the idea of hardcoding
> fixup for the ENCLU opcode, and the basic idea checks out.  The code
> is fairly minimal and doesn't impact the core functionality of the SDK.
> They'd need to redo their trap handling to move it from the signal
> handler to inline, but their stack shenanigans won't be any more broken
> than they already are.

^ permalink raw reply

* Re: RFC: userspace exception fixups
From: Rich Felker @ 2018-11-06 23:17 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Dave Hansen, Christopherson, Sean J, Jann Horn, Linus Torvalds,
	Dave Hansen, Jethro Beekman, Jarkko Sakkinen, Florian Weimer,
	Linux API, X86 ML, linux-arch, LKML, Peter Zijlstra, nhorman,
	npmccallum, Ayoun, Serge, shay.katz-zamir, linux-sgx,
	Andy Shevchenko, Thomas Gleixner
In-Reply-To: <CALCETrWBV=1JbAKYn2Jy2LxkGZQvKRtFRnrWUMoejrwQe73VHw@mail.gmail.com>

On Tue, Nov 06, 2018 at 11:02:11AM -0800, Andy Lutomirski wrote:
> On Tue, Nov 6, 2018 at 10:41 AM Dave Hansen <dave.hansen@intel.com> wrote:
> >
> > On 11/6/18 10:20 AM, Andy Lutomirski wrote:
> > > I almost feel like the right solution is to call into SGX on its own
> > > private stack or maybe even its own private address space.
> >
> > Yeah, I had the same gut feeling.  Couldn't the debugger even treat the
> > enclave like its own "thread" with its own stack and its own set of
> > registers and context?  That seems like a much more workable model than
> > trying to weave it together with the EENTER context.
> 
> So maybe the API should be, roughly
> 
> sgx_exit_reason_t sgx_enter_enclave(pointer_to_enclave, struct
> host_state *state);
> sgx_exit_reason_t sgx_resume_enclave(same args);
> 
> where host_state is something like:
> 
> struct host_state {
>   unsigned long bp, sp, ax, bx, cx, dx, si, di;
> };
> 
> and the values in host_state explicitly have nothing to do with the
> actual host registers.  So, if you want to use the outcall mechanism,
> you'd allocate some memory, point sp to that memory, call
> sgx_enter_enclave(), and then read that memory to do the outcall.
> 
> Actually implementing this would be distinctly nontrivial, and would
> almost certainly need some degree of kernel help to avoid an explosion
> when a signal gets delivered while we have host_state.sp loaded into
> the actual SP register.  Maybe rseq could help with this?
> 
> The ISA here is IMO not well thought through.

Maybe I'm mistaken about some fundamentals here, but my understanding
of SGX is that the whole point is that the host application and the
code running in the enclave are mutually adversarial towards one
another. Do any or all of the proposed protocols here account for this
and fully protect the host application from malicious code in the
enclave? It seems that having control over the register file on exit
from the enclave is fundamentally problematic but I assume there must
be some way I'm missing that this is fixed up.

Rich

^ permalink raw reply

* Re: RFC: userspace exception fixups
From: Sean Christopherson @ 2018-11-06 23:26 UTC (permalink / raw)
  To: Rich Felker
  Cc: Andy Lutomirski, Dave Hansen, Jann Horn, Linus Torvalds,
	Dave Hansen, Jethro Beekman, Jarkko Sakkinen, Florian Weimer,
	Linux API, X86 ML, linux-arch, LKML, Peter Zijlstra, nhorman,
	npmccallum, Ayoun, Serge, shay.katz-zamir, linux-sgx,
	Andy Shevchenko, Thomas Gleixner, Ingo Molnar, Borislav
In-Reply-To: <20181106231730.GR5150@brightrain.aerifal.cx>

On Tue, Nov 06, 2018 at 06:17:30PM -0500, Rich Felker wrote:
> On Tue, Nov 06, 2018 at 11:02:11AM -0800, Andy Lutomirski wrote:
> > On Tue, Nov 6, 2018 at 10:41 AM Dave Hansen <dave.hansen@intel.com> wrote:
> > >
> > > On 11/6/18 10:20 AM, Andy Lutomirski wrote:
> > > > I almost feel like the right solution is to call into SGX on its own
> > > > private stack or maybe even its own private address space.
> > >
> > > Yeah, I had the same gut feeling.  Couldn't the debugger even treat the
> > > enclave like its own "thread" with its own stack and its own set of
> > > registers and context?  That seems like a much more workable model than
> > > trying to weave it together with the EENTER context.
> > 
> > So maybe the API should be, roughly
> > 
> > sgx_exit_reason_t sgx_enter_enclave(pointer_to_enclave, struct
> > host_state *state);
> > sgx_exit_reason_t sgx_resume_enclave(same args);
> > 
> > where host_state is something like:
> > 
> > struct host_state {
> >   unsigned long bp, sp, ax, bx, cx, dx, si, di;
> > };
> > 
> > and the values in host_state explicitly have nothing to do with the
> > actual host registers.  So, if you want to use the outcall mechanism,
> > you'd allocate some memory, point sp to that memory, call
> > sgx_enter_enclave(), and then read that memory to do the outcall.
> > 
> > Actually implementing this would be distinctly nontrivial, and would
> > almost certainly need some degree of kernel help to avoid an explosion
> > when a signal gets delivered while we have host_state.sp loaded into
> > the actual SP register.  Maybe rseq could help with this?
> > 
> > The ISA here is IMO not well thought through.
> 
> Maybe I'm mistaken about some fundamentals here, but my understanding
> of SGX is that the whole point is that the host application and the
> code running in the enclave are mutually adversarial towards one
> another. Do any or all of the proposed protocols here account for this
> and fully protect the host application from malicious code in the
> enclave? It seems that having control over the register file on exit
> from the enclave is fundamentally problematic but I assume there must
> be some way I'm missing that this is fixed up.

SGX provides protections for the enclave but not the other way around.
The kernel has all of its normal non-SGX protections in place, but the
enclave can certainly wreak havoc on its userspace process.  The basic
design idea is that the enclave is a specialized .so that gets extra
security protections but is still effectively part of the overall
application, e.g. it has full access to its host userspace process'
virtual memory.

^ permalink raw reply

* Re: RFC: userspace exception fixups
From: Sean Christopherson @ 2018-11-06 23:35 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Andy Lutomirski, Dave Hansen, Jann Horn, Linus Torvalds,
	Rich Felker, Dave Hansen, Jethro Beekman, Jarkko Sakkinen,
	Florian Weimer, Linux API, X86 ML, linux-arch, LKML,
	Peter Zijlstra, nhorman, npmccallum, Ayoun, Serge,
	shay.katz-zamir, linux-sgx, Andy Shevchenko, Thomas Gleixner
In-Reply-To: <7FF4802E-FBC5-4E6D-A8F6-8A65114F18C7@amacapital.net>

On Tue, Nov 06, 2018 at 03:00:56PM -0800, Andy Lutomirski wrote:
> 
> 
> >> On Nov 6, 2018, at 1:59 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> >> 
> >>> On Tue, 2018-11-06 at 13:41 -0800, Andy Lutomirski wrote:
> >> Sean, how does the current SDK AEX handler decide whether to do
> >> EENTER, ERESUME, or just bail and consider the enclave dead?  It seems
> >> like the *CPU* could give a big hint, but I don't see where there is
> >> any architectural indication of why the AEX code got called or any
> >> obvious way for the user code to know whether the exit was fixed up by
> >> the kernel?
> > 
> > The SDK "unconditionally" does ERESUME at the AEP location, but that's
> > bit misleading because its signal handler may muck with the context's
> > RIP, e.g. to abort the enclave on a fatal fault.
> > 
> > On an event/exception from within an enclave, the event is immediately
> > delivered after loading synthetic state and changing RIP to the AEP.
> > In other words, jamming CPU state is essentially a bunch of vectoring
> > ucode preamble, but from software's perspective it's a normal event
> > that happens to point at the AEP instead of somewhere in the enclave.
> > And because the signals the SDK cares about are all synchronous, the
> > SDK can simply hardcode ERESUME at the AEP since all of the fault logic
> > resides in its signal handler.  IRQs and whatnot simply trampoline back
> > into the enclave.
> > 
> > Userspace can do something funky instead of ERESUME, but only *after*
> > IRET/RSM/VMRESUME has returned to the AEP location, and in Linux's
> > case, after the trap handler has run.
> > 
> > Jumping back a bit, how much do we care about preventing userspace
> > from doing stupid things? 
> 
> My general feeling is that userspace should be allowed to do apparently
> stupid things. For example, as far as the kernel is concerned, Wine and
> DOSEMU are just user programs that do stupid things. Linux generally tries
> to provide a reasonably complete view of architectural behavior. This is
> in contrast to, say, Windows, where IIUC doing an unapproved WRFSBASE May
> cause very odd behavior indeed. So magic fixups that do non-architectural
> things are not so great.

Sorry if I'm beating a dead horse, but what if we only did fixup on ENCLU
with a specific (ignored) prefix pattern?  I.e. effectively make the magic
fixup opt-in, falling back to signals.  Jamming RIP to skip ENCLU isn't
that far off the architecture, e.g. EENTER stuffs RCX with the next RIP so
that the enclave can EEXIT to immediately after the EENTER location.

> (How does the Windows case work?  If there’s an exception after the untrusted
> stack allocation and before EEXIT and SEH tries to handle it, how does the
> unwinder figure out where to start?)

No clue, I'll ask and report back.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox