[intel-sgx-kernel-dev] intel_sgx: add ENCLS macros for returning exact fault vector
diff mbox

Message ID 20170914195418.16672-1-sean.j.christopherson@intel.com
State New
Headers show

Commit Message

Sean Christopherson Sept. 14, 2017, 7:54 p.m. UTC
When trapping EINIT, KVM needs the vector if EINIT faults in order
to inject the correct fault back into the guest.  Add "_raw" suffixed
ENCLS macros that utilize _ASM_EXTABLE_FAULT to return the raw fault
vector to the caller.  The fault vector is shifted into bits 31:16
so that the caller can distinguish between faults and SGX error codes.

Modify the existing __encls_ret and __encls macros to return -EFAULT
if a fault occurs on ENCLS.  This provides userspace with an API that
is more consistent with other IOCTLs, and obviates the need for the
caller to adjust return values, e.g. sgx_ioc_enclave_create no longer
needs to manually return -EFAULT on any __ecreate failure.

Coincidentally, these changes also fix a bug where __encls_ret did
not update EAX after a fault, e.g. __einit would always return '2'
if it faulted.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/asm/sgx.h                | 44 +++++++++++++++----------------
 drivers/platform/x86/intel_sgx/sgx_encl.c |  1 -
 2 files changed, 21 insertions(+), 24 deletions(-)

Comments

Sean Christopherson Sept. 15, 2017, 9:07 p.m. UTC | #1
On Fri, Sep 15, 2017 at 01:41:37PM -0700, Jarkko Sakkinen wrote:
> On Fri, Sep 15, 2017 at 08:22:48PM +0000, Christopherson, Sean J wrote:
> > On Fri, Sep 15, 2017 at 01:11:19PM -0700, Jarkko Sakkinen wrote:
> > > On Fri, Sep 15, 2017 at 07:42:52PM +0000, Christopherson, Sean J wrote:
> > > > On Thu, Sep 14, 2017 at 03:13:33PM -0700, Jarkko Sakkinen wrote:
> > > > > On Thu, Sep 14, 2017 at 12:54:18PM -0700, Sean Christopherson wrote:
> > > > > > When trapping EINIT, KVM needs the vector if EINIT faults in order
> > > > > > to inject the correct fault back into the guest.  Add "_raw" suffixed
> > > > > > ENCLS macros that utilize _ASM_EXTABLE_FAULT to return the raw fault
> > > > > > vector to the caller.  The fault vector is shifted into bits 31:16
> > > > > > so that the caller can distinguish between faults and SGX error codes.
> > > > > >
> > > > > > Modify the existing __encls_ret and __encls macros to return -EFAULT
> > > > > > if a fault occurs on ENCLS.  This provides userspace with an API that
> > > > > > is more consistent with other IOCTLs, and obviates the need for the
> > > > > > caller to adjust return values, e.g. sgx_ioc_enclave_create no longer
> > > > > > needs to manually return -EFAULT on any __ecreate failure.
> > > > > >
> > > > > > Coincidentally, these changes also fix a bug where __encls_ret did
> > > > > > not update EAX after a fault, e.g. __einit would always return '2'
> > > > > > if it faulted.
> > > > > >
> > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > > >
> > > > > Makes sense and in this case the code change could be taken.
> > > > >
> > > > > It would help if you reviewed the actual code you see the issue because
> > > > > now I have to project this back to my patches. And if you have the patch
> > > > > available you can propose sending it. Now it is very unclear where and
> > > > > when you want it. In this case I would have agreed to take your code
> > > > > change.
> > > > >
> > > > > /Jarkko
> > > >
> > > > I submitted patches based on your master branch.  If master is no longer
> > > > valid than please delete the branch or make it explicitly clear that it's
> > > > a stale branch and should not be used.
> > > >
> > > > The patches I submitted have no relation to your LE branch.  The two bug
> > > > fix patches addressed issues that recently bit me but have existed in the
> > > > SGX code since time immemorial.  The EPC bank patch is a v2 revision for
> > > > a months-old patch that I belatedly realized hadn't been accepted.
> > >
> > > The master branch is needed to support out-of-tree driver up
> > > until le branch is upstreamed
> > >
> > > /Jarkko
> >
> > Is master being maintained, i.e. accepting bug patches, or has it been put out
> > to pasture for all intents and purposes?  In other words, should all future
> > comments and/or bug fixes be directed towards the LE patches/branch?
>
> I think the reasonable thing to do is to put a marker in the diffstat
> section of patch if it is a bug fix. I'll apply them to the master
> branch and backport them to the 'le' branch.
>
> For the most part I won't accept new features to the master branch but I
> think that for example these encls changes make sense enough to be put
> there. I'll apply that patch as soon as possible. I'm sorry but I must
> missed the first version somehow.

You didn't miss anything, the EPC bank patch was a v2, not the ENCLS patch.


> The le branch will become the new master after upstreaming. I'll apply
> your encls changes right after upstreaming there so you will get credit
> and the blame for them. It's cutting hairs but with le I'm even more
> conservative on adding new features.

That's fine, though if you take that approach then __encls_ret needs to
be modified in your LE branch to update EAX on a fault.  Returning the
leaf number instead of -1 is a bug that is userspace visible.


> With upstreaming the main blocker is GCC7 compilation issue that I've
> explained earlier. After that is fixed I'm ready to send the patch
> set to LKML.
>
> Does this make sense to you?

Yep.  Hopefully there are no more bugs and this doesn't come up again :)
Jarkko Sakkinen Sept. 15, 2017, 9:30 p.m. UTC | #2
On Fri, Sep 15, 2017 at 09:07:57PM +0000, Christopherson, Sean J wrote:
> > For the most part I won't accept new features to the master branch but I
> > think that for example these encls changes make sense enough to be put
> > there. I'll apply that patch as soon as possible. I'm sorry but I must
> > missed the first version somehow.
> 
> You didn't miss anything, the EPC bank patch was a v2, not the ENCLS patch.

Everything is now anyway in the master branch.

> > The le branch will become the new master after upstreaming. I'll apply
> > your encls changes right after upstreaming there so you will get credit
> > and the blame for them. It's cutting hairs but with le I'm even more
> > conservative on adding new features.
> 
> That's fine, though if you take that approach then __encls_ret needs to
> be modified in your LE branch to update EAX on a fault.  Returning the
> leaf number instead of -1 is a bug that is userspace visible.

You are probably talking about __encls() (in guest __encls_ret() can
fault, which your patch addesses)?

> > With upstreaming the main blocker is GCC7 compilation issue that I've
> > explained earlier. After that is fixed I'm ready to send the patch
> > set to LKML.
> >
> > Does this make sense to you?
> 
> Yep.  Hopefully there are no more bugs and this doesn't come up again :)

If you put a note just before diffstat after the dashes that add this
to the master, I will do it.

I can then do deduction whether it is mandatory for the le branch.

/Jarkko
Sean Christopherson Sept. 15, 2017, 9:42 p.m. UTC | #3
On Fri, Sep 15, 2017 at 02:30:30PM -0700, Jarkko Sakkinen wrote:
> On Fri, Sep 15, 2017 at 09:07:57PM +0000, Christopherson, Sean J wrote:
> > > For the most part I won't accept new features to the master branch but I
> > > think that for example these encls changes make sense enough to be put
> > > there. I'll apply that patch as soon as possible. I'm sorry but I must
> > > missed the first version somehow.
> >
> > You didn't miss anything, the EPC bank patch was a v2, not the ENCLS patch.
>
> Everything is now anyway in the master branch.
>
> > > The le branch will become the new master after upstreaming. I'll apply
> > > your encls changes right after upstreaming there so you will get credit
> > > and the blame for them. It's cutting hairs but with le I'm even more
> > > conservative on adding new features.
> >
> > That's fine, though if you take that approach then __encls_ret needs to
> > be modified in your LE branch to update EAX on a fault.  Returning the
> > leaf number instead of -1 is a bug that is userspace visible.
>
> You are probably talking about __encls() (in guest __encls_ret() can
> fault, which your patch addesses)?

No, I'm talking about __encs_ret.  In your current LE branch, __encls() sets
EAX/RAX to -1 in its .fixup section.  __encls_ret() does not do this, which
results in EAX/RAX retaining its previous value, i.e. the ENCLS leaf, if ENCLS
faults.  For example, if ENCLS[EINIT] faults for whatever reason, then __einit
will ultimately return '2' because __encls_ret() doesn't update EAX/RAX.  This
is extremely confusing for userspace because '2' is also the error code for
SGX_INVALID_ATTRIBUTE, which happens to be a valid error code for EINIT.


> > > With upstreaming the main blocker is GCC7 compilation issue that I've
> > > explained earlier. After that is fixed I'm ready to send the patch
> > > set to LKML.
> > >
> > > Does this make sense to you?
> >
> > Yep.  Hopefully there are no more bugs and this doesn't come up again :)
>
> If you put a note just before diffstat after the dashes that add this
> to the master, I will do it.
>
> I can then do deduction whether it is mandatory for the le branch.
Jarkko Sakkinen Sept. 15, 2017, 10:27 p.m. UTC | #4
On Fri, Sep 15, 2017 at 09:42:31PM +0000, Christopherson, Sean J wrote:
> On Fri, Sep 15, 2017 at 02:30:30PM -0700, Jarkko Sakkinen wrote:
> > On Fri, Sep 15, 2017 at 09:07:57PM +0000, Christopherson, Sean J wrote:
> > > > For the most part I won't accept new features to the master branch but I
> > > > think that for example these encls changes make sense enough to be put
> > > > there. I'll apply that patch as soon as possible. I'm sorry but I must
> > > > missed the first version somehow.
> > >
> > > You didn't miss anything, the EPC bank patch was a v2, not the ENCLS patch.
> >
> > Everything is now anyway in the master branch.
> >
> > > > The le branch will become the new master after upstreaming. I'll apply
> > > > your encls changes right after upstreaming there so you will get credit
> > > > and the blame for them. It's cutting hairs but with le I'm even more
> > > > conservative on adding new features.
> > >
> > > That's fine, though if you take that approach then __encls_ret needs to
> > > be modified in your LE branch to update EAX on a fault.  Returning the
> > > leaf number instead of -1 is a bug that is userspace visible.
> >
> > You are probably talking about __encls() (in guest __encls_ret() can
> > fault, which your patch addesses)?
> 
> No, I'm talking about __encs_ret.  In your current LE branch, __encls() sets
> EAX/RAX to -1 in its .fixup section.  __encls_ret() does not do this, which
> results in EAX/RAX retaining its previous value, i.e. the ENCLS leaf, if ENCLS
> faults.  For example, if ENCLS[EINIT] faults for whatever reason, then __einit
> will ultimately return '2' because __encls_ret() doesn't update EAX/RAX.  This
> is extremely confusing for userspace because '2' is also the error code for
> SGX_INVALID_ATTRIBUTE, which happens to be a valid error code for EINIT.

What is the reason ENCLS[EINIT] when it could fault in a legit case (not
a driver bug)?

It would make sense set it to some sane value in the case of a driver
bug and report a critical error (pr_crit) to the user. I'm just trying
to decipher "whatever case" that's all.

/Jarkko
Jarkko Sakkinen Sept. 16, 2017, 4:43 p.m. UTC | #5
On Fri, Sep 15, 2017 at 03:27:19PM -0700, Jarkko Sakkinen wrote:
> On Fri, Sep 15, 2017 at 09:42:31PM +0000, Christopherson, Sean J wrote:
> > On Fri, Sep 15, 2017 at 02:30:30PM -0700, Jarkko Sakkinen wrote:
> > > On Fri, Sep 15, 2017 at 09:07:57PM +0000, Christopherson, Sean J wrote:
> > > > > For the most part I won't accept new features to the master branch but I
> > > > > think that for example these encls changes make sense enough to be put
> > > > > there. I'll apply that patch as soon as possible. I'm sorry but I must
> > > > > missed the first version somehow.
> > > >
> > > > You didn't miss anything, the EPC bank patch was a v2, not the ENCLS patch.
> > >
> > > Everything is now anyway in the master branch.
> > >
> > > > > The le branch will become the new master after upstreaming. I'll apply
> > > > > your encls changes right after upstreaming there so you will get credit
> > > > > and the blame for them. It's cutting hairs but with le I'm even more
> > > > > conservative on adding new features.
> > > >
> > > > That's fine, though if you take that approach then __encls_ret needs to
> > > > be modified in your LE branch to update EAX on a fault.  Returning the
> > > > leaf number instead of -1 is a bug that is userspace visible.
> > >
> > > You are probably talking about __encls() (in guest __encls_ret() can
> > > fault, which your patch addesses)?
> > 
> > No, I'm talking about __encs_ret.  In your current LE branch, __encls() sets
> > EAX/RAX to -1 in its .fixup section.  __encls_ret() does not do this, which
> > results in EAX/RAX retaining its previous value, i.e. the ENCLS leaf, if ENCLS
> > faults.  For example, if ENCLS[EINIT] faults for whatever reason, then __einit
> > will ultimately return '2' because __encls_ret() doesn't update EAX/RAX.  This
> > is extremely confusing for userspace because '2' is also the error code for
> > SGX_INVALID_ATTRIBUTE, which happens to be a valid error code for EINIT.
> 
> What is the reason ENCLS[EINIT] when it could fault in a legit case (not
> a driver bug)?
> 
> It would make sense set it to some sane value in the case of a driver
> bug and report a critical error (pr_crit) to the user. I'm just trying
> to decipher "whatever case" that's all.
> 
> /Jarkko

This can be postponed up until the driver is upstreamed unless there
is a legit case where it could fault.

/Jarkko
Jarkko Sakkinen Sept. 17, 2017, 4:16 a.m. UTC | #6
On Fri, Sep 15, 2017 at 06:40:05AM -0700, Jarkko Sakkinen wrote:
> > > > > Shall we add "cc" to clobber list as RFLAGS may be updated by
> > > > > ENCLS?

Hey, I think I was too rushy while responsing to these emails in the
middle of the  conference. Calling convention does not require saving
rflags so do you have any specific reason why they should be in the
clobbered list?

It's a macro but still I doubt GCC would do anything wrong here. Please
correct me if I'm missing something here.

However, I still think that volatile can be safely dropped. I have to
test this at home though.

/Jarkko
Jarkko Sakkinen Sept. 17, 2017, 4:20 a.m. UTC | #7
On Fri, Sep 15, 2017 at 02:30:30PM -0700, Jarkko Sakkinen wrote:
> On Fri, Sep 15, 2017 at 09:07:57PM +0000, Christopherson, Sean J wrote:
> > > For the most part I won't accept new features to the master branch but I
> > > think that for example these encls changes make sense enough to be put
> > > there. I'll apply that patch as soon as possible. I'm sorry but I must
> > > missed the first version somehow.
> > 
> > You didn't miss anything, the EPC bank patch was a v2, not the ENCLS patch.
> 
> Everything is now anyway in the master branch.

I took another look at this (was too rushy in the middle of the con).
No one is consuming fault vector. Why is it there?

/Jarkko
Sean Christopherson Sept. 18, 2017, 1:09 p.m. UTC | #8
On Sat, Sep 16, 2017 at 09:43:12AM -0700, Jarkko Sakkinen wrote:
> On Fri, Sep 15, 2017 at 03:27:19PM -0700, Jarkko Sakkinen wrote:
> > On Fri, Sep 15, 2017 at 09:42:31PM +0000, Christopherson, Sean J wrote:
> > > On Fri, Sep 15, 2017 at 02:30:30PM -0700, Jarkko Sakkinen wrote:
> > > > On Fri, Sep 15, 2017 at 09:07:57PM +0000, Christopherson, Sean J wrote:
> > > > > > For the most part I won't accept new features to the master branch but I
> > > > > > think that for example these encls changes make sense enough to be put
> > > > > > there. I'll apply that patch as soon as possible. I'm sorry but I must
> > > > > > missed the first version somehow.
> > > > >
> > > > > You didn't miss anything, the EPC bank patch was a v2, not the ENCLS patch.
> > > >
> > > > Everything is now anyway in the master branch.
> > > >
> > > > > > The le branch will become the new master after upstreaming. I'll apply
> > > > > > your encls changes right after upstreaming there so you will get credit
> > > > > > and the blame for them. It's cutting hairs but with le I'm even more
> > > > > > conservative on adding new features.
> > > > >
> > > > > That's fine, though if you take that approach then __encls_ret needs to
> > > > > be modified in your LE branch to update EAX on a fault.  Returning the
> > > > > leaf number instead of -1 is a bug that is userspace visible.
> > > >
> > > > You are probably talking about __encls() (in guest __encls_ret() can
> > > > fault, which your patch addesses)?
> > >
> > > No, I'm talking about __encs_ret.  In your current LE branch, __encls() sets
> > > EAX/RAX to -1 in its .fixup section.  __encls_ret() does not do this, which
> > > results in EAX/RAX retaining its previous value, i.e. the ENCLS leaf, if ENCLS
> > > faults.  For example, if ENCLS[EINIT] faults for whatever reason, then __einit
> > > will ultimately return '2' because __encls_ret() doesn't update EAX/RAX.  This
> > > is extremely confusing for userspace because '2' is also the error code for
> > > SGX_INVALID_ATTRIBUTE, which happens to be a valid error code for EINIT.
> >
> > What is the reason ENCLS[EINIT] when it could fault in a legit case (not
> > a driver bug)?
> >
> > It would make sense set it to some sane value in the case of a driver
> > bug and report a critical error (pr_crit) to the user. I'm just trying
> > to decipher "whatever case" that's all.
> >
> > /Jarkko
>
> This can be postponed up until the driver is upstreamed unless there
> is a legit case where it could fault.

Any ENCLS instruction will #GP if SGX is not enabled in the feature control
MSR, and the driver does not guarantee that it's enabled for all CPUs.
Jarkko Sakkinen Sept. 18, 2017, 5:53 p.m. UTC | #9
On Mon, Sep 18, 2017 at 01:09:58PM +0000, Christopherson, Sean J wrote:
> On Sat, Sep 16, 2017 at 09:43:12AM -0700, Jarkko Sakkinen wrote:
> > On Fri, Sep 15, 2017 at 03:27:19PM -0700, Jarkko Sakkinen wrote:
> > > On Fri, Sep 15, 2017 at 09:42:31PM +0000, Christopherson, Sean J wrote:
> > > > On Fri, Sep 15, 2017 at 02:30:30PM -0700, Jarkko Sakkinen wrote:
> > > > > On Fri, Sep 15, 2017 at 09:07:57PM +0000, Christopherson, Sean J wrote:
> > > > > > > For the most part I won't accept new features to the master branch but I
> > > > > > > think that for example these encls changes make sense enough to be put
> > > > > > > there. I'll apply that patch as soon as possible. I'm sorry but I must
> > > > > > > missed the first version somehow.
> > > > > >
> > > > > > You didn't miss anything, the EPC bank patch was a v2, not the ENCLS patch.
> > > > >
> > > > > Everything is now anyway in the master branch.
> > > > >
> > > > > > > The le branch will become the new master after upstreaming. I'll apply
> > > > > > > your encls changes right after upstreaming there so you will get credit
> > > > > > > and the blame for them. It's cutting hairs but with le I'm even more
> > > > > > > conservative on adding new features.
> > > > > >
> > > > > > That's fine, though if you take that approach then __encls_ret needs to
> > > > > > be modified in your LE branch to update EAX on a fault.  Returning the
> > > > > > leaf number instead of -1 is a bug that is userspace visible.
> > > > >
> > > > > You are probably talking about __encls() (in guest __encls_ret() can
> > > > > fault, which your patch addesses)?
> > > >
> > > > No, I'm talking about __encs_ret.  In your current LE branch, __encls() sets
> > > > EAX/RAX to -1 in its .fixup section.  __encls_ret() does not do this, which
> > > > results in EAX/RAX retaining its previous value, i.e. the ENCLS leaf, if ENCLS
> > > > faults.  For example, if ENCLS[EINIT] faults for whatever reason, then __einit
> > > > will ultimately return '2' because __encls_ret() doesn't update EAX/RAX.  This
> > > > is extremely confusing for userspace because '2' is also the error code for
> > > > SGX_INVALID_ATTRIBUTE, which happens to be a valid error code for EINIT.
> > >
> > > What is the reason ENCLS[EINIT] when it could fault in a legit case (not
> > > a driver bug)?
> > >
> > > It would make sense set it to some sane value in the case of a driver
> > > bug and report a critical error (pr_crit) to the user. I'm just trying
> > > to decipher "whatever case" that's all.
> > >
> > > /Jarkko
> >
> > This can be postponed up until the driver is upstreamed unless there
> > is a legit case where it could fault.
> 
> Any ENCLS instruction will #GP if SGX is not enabled in the feature control
> MSR, and the driver does not guarantee that it's enabled for all CPUs.

That is a valid point.

Here's a small suggestion for the patch. I think it would cleaner to
have it as follows

1. Add the stuff that you added to '_raw' macros.
2. Have a macro ENCLS_TO_ERR(x) that converts result of the macro
   to a system error code.

This would make it less polluted although call sites have to be
converted to use ENCLS_TO_ERR() .

Can you implement it this way? This is maybe cutting hairs but
ENCLS_TO_ERR could be a separate commit whereas encls changes will
merged to main driver commit.  For ENCLS_TO_ERR you should add
"Suggested-by:" tag.

/Jarkko
Jarkko Sakkinen Sept. 18, 2017, 6:05 p.m. UTC | #10
On Mon, Sep 18, 2017 at 08:53:37PM +0300, Jarkko Sakkinen wrote:
> On Mon, Sep 18, 2017 at 01:09:58PM +0000, Christopherson, Sean J wrote:
> > On Sat, Sep 16, 2017 at 09:43:12AM -0700, Jarkko Sakkinen wrote:
> > > On Fri, Sep 15, 2017 at 03:27:19PM -0700, Jarkko Sakkinen wrote:
> > > > On Fri, Sep 15, 2017 at 09:42:31PM +0000, Christopherson, Sean J wrote:
> > > > > On Fri, Sep 15, 2017 at 02:30:30PM -0700, Jarkko Sakkinen wrote:
> > > > > > On Fri, Sep 15, 2017 at 09:07:57PM +0000, Christopherson, Sean J wrote:
> > > > > > > > For the most part I won't accept new features to the master branch but I
> > > > > > > > think that for example these encls changes make sense enough to be put
> > > > > > > > there. I'll apply that patch as soon as possible. I'm sorry but I must
> > > > > > > > missed the first version somehow.
> > > > > > >
> > > > > > > You didn't miss anything, the EPC bank patch was a v2, not the ENCLS patch.
> > > > > >
> > > > > > Everything is now anyway in the master branch.
> > > > > >
> > > > > > > > The le branch will become the new master after upstreaming. I'll apply
> > > > > > > > your encls changes right after upstreaming there so you will get credit
> > > > > > > > and the blame for them. It's cutting hairs but with le I'm even more
> > > > > > > > conservative on adding new features.
> > > > > > >
> > > > > > > That's fine, though if you take that approach then __encls_ret needs to
> > > > > > > be modified in your LE branch to update EAX on a fault.  Returning the
> > > > > > > leaf number instead of -1 is a bug that is userspace visible.
> > > > > >
> > > > > > You are probably talking about __encls() (in guest __encls_ret() can
> > > > > > fault, which your patch addesses)?
> > > > >
> > > > > No, I'm talking about __encs_ret.  In your current LE branch, __encls() sets
> > > > > EAX/RAX to -1 in its .fixup section.  __encls_ret() does not do this, which
> > > > > results in EAX/RAX retaining its previous value, i.e. the ENCLS leaf, if ENCLS
> > > > > faults.  For example, if ENCLS[EINIT] faults for whatever reason, then __einit
> > > > > will ultimately return '2' because __encls_ret() doesn't update EAX/RAX.  This
> > > > > is extremely confusing for userspace because '2' is also the error code for
> > > > > SGX_INVALID_ATTRIBUTE, which happens to be a valid error code for EINIT.
> > > >
> > > > What is the reason ENCLS[EINIT] when it could fault in a legit case (not
> > > > a driver bug)?
> > > >
> > > > It would make sense set it to some sane value in the case of a driver
> > > > bug and report a critical error (pr_crit) to the user. I'm just trying
> > > > to decipher "whatever case" that's all.
> > > >
> > > > /Jarkko
> > >
> > > This can be postponed up until the driver is upstreamed unless there
> > > is a legit case where it could fault.
> > 
> > Any ENCLS instruction will #GP if SGX is not enabled in the feature control
> > MSR, and the driver does not guarantee that it's enabled for all CPUs.
> 
> That is a valid point.
> 
> Here's a small suggestion for the patch. I think it would cleaner to
> have it as follows
> 
> 1. Add the stuff that you added to '_raw' macros.
> 2. Have a macro ENCLS_TO_ERR(x) that converts result of the macro
>    to a system error code.
> 
> This would make it less polluted although call sites have to be
> converted to use ENCLS_TO_ERR() .
> 
> Can you implement it this way? This is maybe cutting hairs but
> ENCLS_TO_ERR could be a separate commit whereas encls changes will
> merged to main driver commit.  For ENCLS_TO_ERR you should add
> "Suggested-by:" tag.
> 
> /Jarkko

You can just send one commit and I'll chop it in the way I see proper :-)

/jarkko
Jarkko Sakkinen Sept. 18, 2017, 6:27 p.m. UTC | #11
On Mon, Sep 18, 2017 at 09:05:13PM +0300, Jarkko Sakkinen wrote:
> On Mon, Sep 18, 2017 at 08:53:37PM +0300, Jarkko Sakkinen wrote:
> > On Mon, Sep 18, 2017 at 01:09:58PM +0000, Christopherson, Sean J wrote:
> > > On Sat, Sep 16, 2017 at 09:43:12AM -0700, Jarkko Sakkinen wrote:
> > > > On Fri, Sep 15, 2017 at 03:27:19PM -0700, Jarkko Sakkinen wrote:
> > > > > On Fri, Sep 15, 2017 at 09:42:31PM +0000, Christopherson, Sean J wrote:
> > > > > > On Fri, Sep 15, 2017 at 02:30:30PM -0700, Jarkko Sakkinen wrote:
> > > > > > > On Fri, Sep 15, 2017 at 09:07:57PM +0000, Christopherson, Sean J wrote:
> > > > > > > > > For the most part I won't accept new features to the master branch but I
> > > > > > > > > think that for example these encls changes make sense enough to be put
> > > > > > > > > there. I'll apply that patch as soon as possible. I'm sorry but I must
> > > > > > > > > missed the first version somehow.
> > > > > > > >
> > > > > > > > You didn't miss anything, the EPC bank patch was a v2, not the ENCLS patch.
> > > > > > >
> > > > > > > Everything is now anyway in the master branch.
> > > > > > >
> > > > > > > > > The le branch will become the new master after upstreaming. I'll apply
> > > > > > > > > your encls changes right after upstreaming there so you will get credit
> > > > > > > > > and the blame for them. It's cutting hairs but with le I'm even more
> > > > > > > > > conservative on adding new features.
> > > > > > > >
> > > > > > > > That's fine, though if you take that approach then __encls_ret needs to
> > > > > > > > be modified in your LE branch to update EAX on a fault.  Returning the
> > > > > > > > leaf number instead of -1 is a bug that is userspace visible.
> > > > > > >
> > > > > > > You are probably talking about __encls() (in guest __encls_ret() can
> > > > > > > fault, which your patch addesses)?
> > > > > >
> > > > > > No, I'm talking about __encs_ret.  In your current LE branch, __encls() sets
> > > > > > EAX/RAX to -1 in its .fixup section.  __encls_ret() does not do this, which
> > > > > > results in EAX/RAX retaining its previous value, i.e. the ENCLS leaf, if ENCLS
> > > > > > faults.  For example, if ENCLS[EINIT] faults for whatever reason, then __einit
> > > > > > will ultimately return '2' because __encls_ret() doesn't update EAX/RAX.  This
> > > > > > is extremely confusing for userspace because '2' is also the error code for
> > > > > > SGX_INVALID_ATTRIBUTE, which happens to be a valid error code for EINIT.
> > > > >
> > > > > What is the reason ENCLS[EINIT] when it could fault in a legit case (not
> > > > > a driver bug)?
> > > > >
> > > > > It would make sense set it to some sane value in the case of a driver
> > > > > bug and report a critical error (pr_crit) to the user. I'm just trying
> > > > > to decipher "whatever case" that's all.
> > > > >
> > > > > /Jarkko
> > > >
> > > > This can be postponed up until the driver is upstreamed unless there
> > > > is a legit case where it could fault.
> > > 
> > > Any ENCLS instruction will #GP if SGX is not enabled in the feature control
> > > MSR, and the driver does not guarantee that it's enabled for all CPUs.
> > 
> > That is a valid point.
> > 
> > Here's a small suggestion for the patch. I think it would cleaner to
> > have it as follows
> > 
> > 1. Add the stuff that you added to '_raw' macros.
> > 2. Have a macro ENCLS_TO_ERR(x) that converts result of the macro
> >    to a system error code.
> > 
> > This would make it less polluted although call sites have to be
> > converted to use ENCLS_TO_ERR() .
> > 
> > Can you implement it this way? This is maybe cutting hairs but
> > ENCLS_TO_ERR could be a separate commit whereas encls changes will
> > merged to main driver commit.  For ENCLS_TO_ERR you should add
> > "Suggested-by:" tag.
> > 
> > /Jarkko
> 
> You can just send one commit and I'll chop it in the way I see proper :-)
> 
> /jarkko

Please also add yourself to the authors list of sgx.h as this is so
essential change how the macros work. After the driver is upstreamed
those lists will never be updated since commit log will take care of
that but for the initial versions they do make sense. [1]

[1] I will not update them for minor fixes etc, only for "architecture
defining" changes like this.

/Jarkko
Jethro Beekman Sept. 22, 2017, 8:04 a.m. UTC | #12
On 2017-09-15 13:11, Jarkko Sakkinen wrote:
> The master branch is needed to support out-of-tree driver up
> until le branch is upstreamed

Jarkko, are you still maintaining the master branch? The current version doesn't
work because there is no way to supply EINITTOKEN.

> /Jarkko

Jethro
Jarkko Sakkinen Sept. 25, 2017, 12:47 a.m. UTC | #13
On Fri, Sep 22, 2017 at 01:04:13AM -0700, Jethro Beekman wrote:
> On 2017-09-15 13:11, Jarkko Sakkinen wrote:
> > The master branch is needed to support out-of-tree driver up
> > until le branch is upstreamed
> 
> Jarkko, are you still maintaining the master branch? The current version doesn't
> work because there is no way to supply EINITTOKEN.
> 
> > /Jarkko
> 
> Jethro

My understanding is that out-of-tree maintainers will patch that. It
includes subset of commits of le branch that they need.

/Jarkko

Patch
diff mbox

diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
index 541abddab6c7..38a088c28d69 100644
--- a/arch/x86/include/asm/sgx.h
+++ b/arch/x86/include/asm/sgx.h
@@ -84,24 +84,27 @@  enum sgx_commands {
 	EMODT	= 0xF,
 };
 
-#define __encls_ret(rax, rbx, rcx, rdx)			\
+#define IS_ENCLS_FAULT(r) ((r) & 0xffff0000)
+#define ENCLS_FAULT_VECTOR(r) ((r) >> 16)
+
+#define __encls_ret_raw(rax, rbx, rcx, rdx)		\
 	({						\
 	int ret;					\
 	asm volatile(					\
 	"1: .byte 0x0f, 0x01, 0xcf;\n\t"		\
 	"2:\n"						\
 	".section .fixup,\"ax\"\n"			\
-	"3: jmp 2b\n"					\
+	"3: shll $16,%%eax\n"				\
+	"   jmp 2b\n"					\
 	".previous\n"					\
-	_ASM_EXTABLE(1b, 3b)				\
+	_ASM_EXTABLE_FAULT(1b, 3b)			\
 	: "=a"(ret)					\
 	: "a"(rax), "b"(rbx), "c"(rcx), "d"(rdx)	\
 	: "memory");					\
 	ret;						\
 	})
 
-#ifdef CONFIG_X86_64
-#define __encls(rax, rbx, rcx, rdx...)			\
+#define __encls_raw(rax, rbx, rcx, rdx...)		\
 	({						\
 	int ret;					\
 	asm volatile(					\
@@ -109,36 +112,31 @@  enum sgx_commands {
 	"   xor %%eax,%%eax;\n"				\
 	"2:\n"						\
 	".section .fixup,\"ax\"\n"			\
-	"3: movq $-1,%%rax\n"				\
+	"3: shll $16,%%eax\n"				\
 	"   jmp 2b\n"					\
 	".previous\n"					\
-	_ASM_EXTABLE(1b, 3b)				\
+	_ASM_EXTABLE_FAULT(1b, 3b)			\
 	: "=a"(ret), "=b"(rbx), "=c"(rcx)		\
 	: "a"(rax), "b"(rbx), "c"(rcx), rdx		\
 	: "memory");					\
 	ret;						\
 	})
-#else
+
+#define __encls_ret(rax, rbx, rcx, rdx)			\
+	({						\
+	int ret = __encls_ret_raw(rax, rbx, rcx, rdx);	\
+	ret = IS_ENCLS_FAULT(ret) ? -EFAULT : ret;	\
+	ret;						\
+	})
+
 #define __encls(rax, rbx, rcx, rdx...)			\
 	({						\
-	int ret;					\
-	asm volatile(					\
-	"1: .byte 0x0f, 0x01, 0xcf;\n\t"		\
-	"   xor %%eax,%%eax;\n"				\
-	"2:\n"						\
-	".section .fixup,\"ax\"\n"			\
-	"3: mov $-1,%%eax\n"				\
-	"   jmp 2b\n"					\
-	".previous\n"					\
-	_ASM_EXTABLE(1b, 3b)				\
-	: "=a"(ret), "=b"(rbx), "=c"(rcx)		\
-	: "a"(rax), "b"(rbx), "c"(rcx), rdx		\
-	: "memory");					\
+	int ret = __encls_raw(rax, rbx, rcx, rdx);	\
+	ret = IS_ENCLS_FAULT(ret) ? -EFAULT : ret;	\
 	ret;						\
 	})
-#endif
 
-static inline unsigned long __ecreate(struct sgx_pageinfo *pginfo, void *secs)
+static inline int __ecreate(struct sgx_pageinfo *pginfo, void *secs)
 {
 	return __encls(ECREATE, pginfo, secs, "d"(0));
 }
diff --git a/drivers/platform/x86/intel_sgx/sgx_encl.c b/drivers/platform/x86/intel_sgx/sgx_encl.c
index cde732bb6e83..9451369ae43a 100644
--- a/drivers/platform/x86/intel_sgx/sgx_encl.c
+++ b/drivers/platform/x86/intel_sgx/sgx_encl.c
@@ -524,7 +524,6 @@  int sgx_encl_create(struct sgx_secs *secs)
 
 	if (ret) {
 		sgx_dbg(encl, "ECREATE returned %ld\n", ret);
-		ret = -EFAULT;
 		goto out;
 	}