Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Borislav Petkov]

On Tue, Oct 27, 2020 at 08:20:00AM -0700, Dave Hansen wrote:
> I can't think of a *lot* of spots where we have sanity checks like this
> for memory.  We have cgroups and the overcommit limits.  But, in
> general, folks can allocate as much memory as they want until
> allocations start to fail.
>
> Should SGX be any different?
> 
> If we had a sanity check that said, "you can only allocate 1/2 of
> enclave memory", wouldn't that just make somebody mad because they want
> one big enclave?
>
> Or, do you just want a sanity check to see if, up front, the user is
> asking for more enclave memory than there is on the *whole* system?
> That's also sane, but it doesn't take overcommit into account.  That's
> why, for instance, we have vm.overcommit_ratio for normal memory.

Yeah, you're making sense and there's really no need for SGX to be any
different. Especially since users are already familiar the "policy" of
failing allocations when too much memory requested. :-)

> BTW, I think we all agree that a cgroup controller for enclave memory is
> going to be needed eventually.

Right.

Thx.

-- 
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Dave Hansen]

On 10/27/20 3:05 AM, Borislav Petkov wrote:
> On Mon, Oct 26, 2020 at 02:26:13PM -0700, Dave Hansen wrote:
>> What were you concerned about here?  Was it how long the syscall could
>> take, or that one user could exhaust all the enclave memory in one call?
> More the latter. And generally, to have a sanity-check on all requests
> coming from luserspace.

I can't think of a *lot* of spots where we have sanity checks like this
for memory.  We have cgroups and the overcommit limits.  But, in
general, folks can allocate as much memory as they want until
allocations start to fail.

Should SGX be any different?

If we had a sanity check that said, "you can only allocate 1/2 of
enclave memory", wouldn't that just make somebody mad because they want
one big enclave?

Or, do you just want a sanity check to see if, up front, the user is
asking for more enclave memory than there is on the *whole* system?
That's also sane, but it doesn't take overcommit into account.  That's
why, for instance, we have vm.overcommit_ratio for normal memory.

BTW, I think we all agree that a cgroup controller for enclave memory is
going to be needed eventually.

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Borislav Petkov]

On Mon, Oct 26, 2020 at 02:26:13PM -0700, Dave Hansen wrote:
> What were you concerned about here?  Was it how long the syscall could
> take, or that one user could exhaust all the enclave memory in one call?

More the latter. And generally, to have a sanity-check on all requests
coming from luserspace.

> Some later versions of this patch have a 1MB limit per to reduce how
> long each SGX_IOC_ENCLAVE_ADD_PAGES call spends in the kernel.  But, I'm
> not _sure_ that's what you were intending.

Yeah, that was not my main goal - rather to sanity-check user input and
impose a sane limit.

Thx.

-- 
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Jarkko Sakkinen]

On Mon, Oct 26, 2020 at 02:26:13PM -0700, Dave Hansen wrote:
> On 6/26/20 8:34 AM, Borislav Petkov wrote:
> >> +  if (!(atomic_read(>flags) & SGX_ENCL_CREATED))
> >> +  return -EINVAL;
> >> +
> >> +  if (copy_from_user(, arg, sizeof(addp)))
> >> +  return -EFAULT;
> >> +
> >> +  if (!IS_ALIGNED(addp.offset, PAGE_SIZE) ||
> >> +  !IS_ALIGNED(addp.src, PAGE_SIZE))
> >> +  return -EINVAL;
> >> +
> >> +  if (!(access_ok(addp.src, PAGE_SIZE)))
> >> +  return -EFAULT;
> >> +
> >> +  if (addp.length & (PAGE_SIZE - 1))
> >> +  return -EINVAL;
> > How many pages are allowed? Unlimited? I'm hoping some limits are
> > checked somewhere...
> 
> What were you concerned about here?  Was it how long the syscall could
> take, or that one user could exhaust all the enclave memory in one call?
> 
> Some later versions of this patch have a 1MB limit per to reduce how
> long each SGX_IOC_ENCLAVE_ADD_PAGES call spends in the kernel.  But, I'm
> not _sure_ that's what you were intending.

The loop does check for pending signals, i.e. it is possible to
interrupt it.

/Jarkko

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Dave Hansen]

On 6/26/20 8:34 AM, Borislav Petkov wrote:
>> +if (!(atomic_read(>flags) & SGX_ENCL_CREATED))
>> +return -EINVAL;
>> +
>> +if (copy_from_user(, arg, sizeof(addp)))
>> +return -EFAULT;
>> +
>> +if (!IS_ALIGNED(addp.offset, PAGE_SIZE) ||
>> +!IS_ALIGNED(addp.src, PAGE_SIZE))
>> +return -EINVAL;
>> +
>> +if (!(access_ok(addp.src, PAGE_SIZE)))
>> +return -EFAULT;
>> +
>> +if (addp.length & (PAGE_SIZE - 1))
>> +return -EINVAL;
> How many pages are allowed? Unlimited? I'm hoping some limits are
> checked somewhere...

What were you concerned about here?  Was it how long the syscall could
take, or that one user could exhaust all the enclave memory in one call?

Some later versions of this patch have a 1MB limit per to reduce how
long each SGX_IOC_ENCLAVE_ADD_PAGES call spends in the kernel.  But, I'm
not _sure_ that's what you were intending.

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Jarkko Sakkinen]

On Tue, Sep 01, 2020 at 10:06:32PM -0500, Haitao Huang wrote:
> On Fri, 03 Jul 2020 22:31:10 -0500, Jarkko Sakkinen
>  wrote:
> 
> > On Wed, Jul 01, 2020 at 08:59:02PM -0700, Sean Christopherson wrote:
> > > On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> > > > +static int sgx_validate_secs(const struct sgx_secs *secs,
> > > > +unsigned long ssaframesize)
> > > > +{
> > > > +   if (secs->size < (2 * PAGE_SIZE) || !is_power_of_2(secs->size))
> > > > +   return -EINVAL;
> > > > +
> > > > +   if (secs->base & (secs->size - 1))
> > > > +   return -EINVAL;
> > > > +
> > > > +   if (secs->miscselect & sgx_misc_reserved_mask ||
> > > > +   secs->attributes & sgx_attributes_reserved_mask ||
> > > > +   secs->xfrm & sgx_xfrm_reserved_mask)
> > > > +   return -EINVAL;
> > > > +
> > > > +   if (secs->attributes & SGX_ATTR_MODE64BIT) {
> > > > +   if (secs->size > sgx_encl_size_max_64)
> > > > +   return -EINVAL;
> > > > +   } else if (secs->size > sgx_encl_size_max_32)
> > > > +   return -EINVAL;
> > > 
> > > These should be >=, not >, the SDM uses one of those fancy ≥ ligatures.
> > > 
> > > Internal versions use more obvious pseudocode, e.g.:
> > > 
> > > if ((DS:TMP_SECS.ATTRIBUTES.MODE64BIT = 1) AND
> > > (DS:TMP_SECS.SIZE AND (~((1 << CPUID.18.0:EDX[15:8]) – 1)))
> > > {
> > > #GP(0);
> > 
> > Updated as:
> > 
> > static int sgx_validate_secs(const struct sgx_secs *secs)
> > {
> > u64 max_size = (secs->attributes & SGX_ATTR_MODE64BIT) ?
> >sgx_encl_size_max_64 : sgx_encl_size_max_32;
> > 
> > if (secs->size < (2 * PAGE_SIZE) || !is_power_of_2(secs->size))
> > return -EINVAL;
> > 
> > if (secs->base & (secs->size - 1))
> > return -EINVAL;
> > 
> > if (secs->miscselect & sgx_misc_reserved_mask ||
> > secs->attributes & sgx_attributes_reserved_mask ||
> > secs->xfrm & sgx_xfrm_reserved_mask)
> > return -EINVAL;
> > 
> > if (secs->size >= max_size)
> > return -EINVAL;
> > 
> 
> This should be > not >=. Issue raised and fixed by Fábio Silva for ported
> patches for OOT SGX support:
> https://github.com/intel/SGXDataCenterAttestationPrimitives/pull/123
> 
> I tested and verified with Intel arch, the comparison indeed should be >.
> 
> Thanks
> Haitao

Thans a lot!

I added this changelog entry to the v37 log:

* Fixed off-by-one error in a size calculation:
  
https://github.com/intel/SGXDataCenterAttestationPrimitives/commit/e44cc238becf584cc079aef40b557c6af9a03f38

Given the Boris' earlier feedback I xref every changelog
entry in v37 changelog. Then it is also less time consuming
to spot if something is missing.

/Jarkko

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Haitao Huang]

On Wed, 02 Sep 2020 11:10:12 -0500, Sean Christopherson  
 wrote:



On Tue, Sep 01, 2020 at 10:06:32PM -0500, Haitao Huang wrote:

On Fri, 03 Jul 2020 22:31:10 -0500, Jarkko Sakkinen
 wrote:

> On Wed, Jul 01, 2020 at 08:59:02PM -0700, Sean Christopherson wrote:
> > On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> > > +static int sgx_validate_secs(const struct sgx_secs *secs,
> > > + unsigned long ssaframesize)
> > > +{
> > > +if (secs->size < (2 * PAGE_SIZE) || !is_power_of_2(secs->size))
> > > +return -EINVAL;
> > > +
> > > +if (secs->base & (secs->size - 1))
> > > +return -EINVAL;
> > > +
> > > +if (secs->miscselect & sgx_misc_reserved_mask ||
> > > +secs->attributes & sgx_attributes_reserved_mask ||
> > > +secs->xfrm & sgx_xfrm_reserved_mask)
> > > +return -EINVAL;
> > > +
> > > +if (secs->attributes & SGX_ATTR_MODE64BIT) {
> > > +if (secs->size > sgx_encl_size_max_64)
> > > +return -EINVAL;
> > > +} else if (secs->size > sgx_encl_size_max_32)
> > > +return -EINVAL;
> >
> > These should be >=, not >, the SDM uses one of those fancy ≥  
ligatures.

> >
> > Internal versions use more obvious pseudocode, e.g.:
> >
> > if ((DS:TMP_SECS.ATTRIBUTES.MODE64BIT = 1) AND
> > (DS:TMP_SECS.SIZE AND (~((1 << CPUID.18.0:EDX[15:8]) – 1)))
> > {
> > #GP(0);
>
> Updated as:
>
> static int sgx_validate_secs(const struct sgx_secs *secs)
> {
>u64 max_size = (secs->attributes & SGX_ATTR_MODE64BIT) ?
>   sgx_encl_size_max_64 : sgx_encl_size_max_32;
>
>if (secs->size < (2 * PAGE_SIZE) || !is_power_of_2(secs->size))
>return -EINVAL;
>
>if (secs->base & (secs->size - 1))
>return -EINVAL;
>
>if (secs->miscselect & sgx_misc_reserved_mask ||
>secs->attributes & sgx_attributes_reserved_mask ||
>secs->xfrm & sgx_xfrm_reserved_mask)
>return -EINVAL;
>
>if (secs->size >= max_size)
>return -EINVAL;
>

This should be > not >=. Issue raised and fixed by Fábio Silva for  
ported

patches for OOT SGX support:
https://github.com/intel/SGXDataCenterAttestationPrimitives/pull/123

I tested and verified with Intel arch, the comparison indeed should be  
>.


And this is a confirmed SDM bug, correct?


yes, the pseudo code for ECREATE is inaccurate and inconsistent with the  
CPUID spec. The latter is correct.

Haitao

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Sean Christopherson]

On Tue, Sep 01, 2020 at 10:06:32PM -0500, Haitao Huang wrote:
> On Fri, 03 Jul 2020 22:31:10 -0500, Jarkko Sakkinen
>  wrote:
> 
> > On Wed, Jul 01, 2020 at 08:59:02PM -0700, Sean Christopherson wrote:
> > > On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> > > > +static int sgx_validate_secs(const struct sgx_secs *secs,
> > > > +unsigned long ssaframesize)
> > > > +{
> > > > +   if (secs->size < (2 * PAGE_SIZE) || !is_power_of_2(secs->size))
> > > > +   return -EINVAL;
> > > > +
> > > > +   if (secs->base & (secs->size - 1))
> > > > +   return -EINVAL;
> > > > +
> > > > +   if (secs->miscselect & sgx_misc_reserved_mask ||
> > > > +   secs->attributes & sgx_attributes_reserved_mask ||
> > > > +   secs->xfrm & sgx_xfrm_reserved_mask)
> > > > +   return -EINVAL;
> > > > +
> > > > +   if (secs->attributes & SGX_ATTR_MODE64BIT) {
> > > > +   if (secs->size > sgx_encl_size_max_64)
> > > > +   return -EINVAL;
> > > > +   } else if (secs->size > sgx_encl_size_max_32)
> > > > +   return -EINVAL;
> > > 
> > > These should be >=, not >, the SDM uses one of those fancy ≥ ligatures.
> > > 
> > > Internal versions use more obvious pseudocode, e.g.:
> > > 
> > > if ((DS:TMP_SECS.ATTRIBUTES.MODE64BIT = 1) AND
> > > (DS:TMP_SECS.SIZE AND (~((1 << CPUID.18.0:EDX[15:8]) – 1)))
> > > {
> > > #GP(0);
> > 
> > Updated as:
> > 
> > static int sgx_validate_secs(const struct sgx_secs *secs)
> > {
> > u64 max_size = (secs->attributes & SGX_ATTR_MODE64BIT) ?
> >sgx_encl_size_max_64 : sgx_encl_size_max_32;
> > 
> > if (secs->size < (2 * PAGE_SIZE) || !is_power_of_2(secs->size))
> > return -EINVAL;
> > 
> > if (secs->base & (secs->size - 1))
> > return -EINVAL;
> > 
> > if (secs->miscselect & sgx_misc_reserved_mask ||
> > secs->attributes & sgx_attributes_reserved_mask ||
> > secs->xfrm & sgx_xfrm_reserved_mask)
> > return -EINVAL;
> > 
> > if (secs->size >= max_size)
> > return -EINVAL;
> > 
> 
> This should be > not >=. Issue raised and fixed by Fábio Silva for ported
> patches for OOT SGX support:
> https://github.com/intel/SGXDataCenterAttestationPrimitives/pull/123
> 
> I tested and verified with Intel arch, the comparison indeed should be >.

And this is a confirmed SDM bug, correct?

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Haitao Huang]

On Fri, 03 Jul 2020 22:31:10 -0500, Jarkko Sakkinen  
 wrote:



On Wed, Jul 01, 2020 at 08:59:02PM -0700, Sean Christopherson wrote:

On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> +static int sgx_validate_secs(const struct sgx_secs *secs,
> +   unsigned long ssaframesize)
> +{
> +  if (secs->size < (2 * PAGE_SIZE) || !is_power_of_2(secs->size))
> +  return -EINVAL;
> +
> +  if (secs->base & (secs->size - 1))
> +  return -EINVAL;
> +
> +  if (secs->miscselect & sgx_misc_reserved_mask ||
> +  secs->attributes & sgx_attributes_reserved_mask ||
> +  secs->xfrm & sgx_xfrm_reserved_mask)
> +  return -EINVAL;
> +
> +  if (secs->attributes & SGX_ATTR_MODE64BIT) {
> +  if (secs->size > sgx_encl_size_max_64)
> +  return -EINVAL;
> +  } else if (secs->size > sgx_encl_size_max_32)
> +  return -EINVAL;

These should be >=, not >, the SDM uses one of those fancy ≥ ligatures.

Internal versions use more obvious pseudocode, e.g.:

if ((DS:TMP_SECS.ATTRIBUTES.MODE64BIT = 1) AND
(DS:TMP_SECS.SIZE AND (~((1 << CPUID.18.0:EDX[15:8]) – 1)))
{
#GP(0);


Updated as:

static int sgx_validate_secs(const struct sgx_secs *secs)
{
u64 max_size = (secs->attributes & SGX_ATTR_MODE64BIT) ?
   sgx_encl_size_max_64 : sgx_encl_size_max_32;

if (secs->size < (2 * PAGE_SIZE) || !is_power_of_2(secs->size))
return -EINVAL;

if (secs->base & (secs->size - 1))
return -EINVAL;

if (secs->miscselect & sgx_misc_reserved_mask ||
secs->attributes & sgx_attributes_reserved_mask ||
secs->xfrm & sgx_xfrm_reserved_mask)
return -EINVAL;

if (secs->size >= max_size)
return -EINVAL;



This should be > not >=. Issue raised and fixed by Fábio Silva for ported  
patches for OOT SGX support:  
https://github.com/intel/SGXDataCenterAttestationPrimitives/pull/123


I tested and verified with Intel arch, the comparison indeed should be >.

Thanks
Haitao

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Jarkko Sakkinen]

On Mon, Jul 06, 2020 at 06:38:47PM -0700, Sean Christopherson wrote:
> On Sat, Jul 04, 2020 at 04:43:49AM +0300, Jarkko Sakkinen wrote:
> > On Mon, Jun 29, 2020 at 08:27:19AM -0700, Sean Christopherson wrote:
> > > On Sat, Jun 27, 2020 at 07:43:35PM +0200, Borislav Petkov wrote:
> > > > And you could do similar sanity checks in the other ioctl functions.
> > > 
> > > Ya, as above, SGX_ENCL_INITIALIZED can be checked here.
> > > 
> > > SGX_ENCL_DEAD is actually already checked in in the top level sgx_ioctl(),
> > > i.e. the check in sgx_encl_add_page() can technically be flat out dropped.
> > > 
> > > I say "technically" because I'm a bit torn over SGX_ENCL_DEAD; encl->lock
> > > must be held to SGX_ENCL_DEAD (the page fault and reclaim flows rely on
> > > this), but as it stands today only ioctl() paths (guarded by 
> > > SGX_ENCL_IOCTL)
> > > and sgx_release() (makes the ioctls() unreachable) set SGX_ENCL_DEAD.
> > > 
> > > So it's safe to check SGX_ENCL_DEAD from ioctl() context without holding
> > > encl->lock, at least in the current code base, but it feels weird/sketchy.
> > > 
> > > In the end I don't think I have a strong opinion.  Removing the 
> > > technically
> > > unnecessary DEAD check in sgx_encl_add_page() is the simplest change, so 
> > > it
> > > may make sense to do that and nothing more for initial upstreaming.  Long
> > > term, I fully expect we'll add paths that set SGX_ENCL_DEAD outside of
> > > ioctl() context, e.g. to handle EPC OOM, but it wouldn't be a bad thing to
> > > have a standalone commit in a future series to add DEAD checks (under
> > > encl->lock) in the ADD and INIT flows.
> > 
> > AFAIK nonne of th ioctl's should not need SGX_ENCL_DEAD check.
> 
> I can't tell if the double negative was intended, but I took a peek at your
> current master and see that you removed the SGX_ENCL_DEAD check in
> sgx_ioctl().  That check needs to stay, e.g. if EEXTEND fails we absolutely
> need to prevent any further operations on the enclave.
> 
> The above was calling out that additional checks on SGX_ENCL_DEAD after
> acquiring encl->lock are not necessary because SGX_ENCL_DEAD can only be
> set when the ioctls() are no longer reachable or from within an ioctl(),
> which provides exclusivity via SGX_ENCL_IOCTL.

Got it.

/Jarkko

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Sean Christopherson]

On Sat, Jul 04, 2020 at 04:43:49AM +0300, Jarkko Sakkinen wrote:
> On Mon, Jun 29, 2020 at 08:27:19AM -0700, Sean Christopherson wrote:
> > On Sat, Jun 27, 2020 at 07:43:35PM +0200, Borislav Petkov wrote:
> > > And you could do similar sanity checks in the other ioctl functions.
> > 
> > Ya, as above, SGX_ENCL_INITIALIZED can be checked here.
> > 
> > SGX_ENCL_DEAD is actually already checked in in the top level sgx_ioctl(),
> > i.e. the check in sgx_encl_add_page() can technically be flat out dropped.
> > 
> > I say "technically" because I'm a bit torn over SGX_ENCL_DEAD; encl->lock
> > must be held to SGX_ENCL_DEAD (the page fault and reclaim flows rely on
> > this), but as it stands today only ioctl() paths (guarded by SGX_ENCL_IOCTL)
> > and sgx_release() (makes the ioctls() unreachable) set SGX_ENCL_DEAD.
> > 
> > So it's safe to check SGX_ENCL_DEAD from ioctl() context without holding
> > encl->lock, at least in the current code base, but it feels weird/sketchy.
> > 
> > In the end I don't think I have a strong opinion.  Removing the technically
> > unnecessary DEAD check in sgx_encl_add_page() is the simplest change, so it
> > may make sense to do that and nothing more for initial upstreaming.  Long
> > term, I fully expect we'll add paths that set SGX_ENCL_DEAD outside of
> > ioctl() context, e.g. to handle EPC OOM, but it wouldn't be a bad thing to
> > have a standalone commit in a future series to add DEAD checks (under
> > encl->lock) in the ADD and INIT flows.
> 
> AFAIK nonne of th ioctl's should not need SGX_ENCL_DEAD check.

I can't tell if the double negative was intended, but I took a peek at your
current master and see that you removed the SGX_ENCL_DEAD check in
sgx_ioctl().  That check needs to stay, e.g. if EEXTEND fails we absolutely
need to prevent any further operations on the enclave.

The above was calling out that additional checks on SGX_ENCL_DEAD after
acquiring encl->lock are not necessary because SGX_ENCL_DEAD can only be
set when the ioctls() are no longer reachable or from within an ioctl(),
which provides exclusivity via SGX_ENCL_IOCTL.

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Jarkko Sakkinen]

On Wed, Jul 01, 2020 at 08:59:02PM -0700, Sean Christopherson wrote:
> On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> > +static int sgx_validate_secs(const struct sgx_secs *secs,
> > +unsigned long ssaframesize)
> > +{
> > +   if (secs->size < (2 * PAGE_SIZE) || !is_power_of_2(secs->size))
> > +   return -EINVAL;
> > +
> > +   if (secs->base & (secs->size - 1))
> > +   return -EINVAL;
> > +
> > +   if (secs->miscselect & sgx_misc_reserved_mask ||
> > +   secs->attributes & sgx_attributes_reserved_mask ||
> > +   secs->xfrm & sgx_xfrm_reserved_mask)
> > +   return -EINVAL;
> > +
> > +   if (secs->attributes & SGX_ATTR_MODE64BIT) {
> > +   if (secs->size > sgx_encl_size_max_64)
> > +   return -EINVAL;
> > +   } else if (secs->size > sgx_encl_size_max_32)
> > +   return -EINVAL;
> 
> These should be >=, not >, the SDM uses one of those fancy ≥ ligatures.
> 
> Internal versions use more obvious pseudocode, e.g.:
> 
> if ((DS:TMP_SECS.ATTRIBUTES.MODE64BIT = 1) AND
> (DS:TMP_SECS.SIZE AND (~((1 << CPUID.18.0:EDX[15:8]) – 1)))
> {
> #GP(0);

Updated as:

static int sgx_validate_secs(const struct sgx_secs *secs)
{
u64 max_size = (secs->attributes & SGX_ATTR_MODE64BIT) ?
   sgx_encl_size_max_64 : sgx_encl_size_max_32;

if (secs->size < (2 * PAGE_SIZE) || !is_power_of_2(secs->size))
return -EINVAL;

if (secs->base & (secs->size - 1))
return -EINVAL;

if (secs->miscselect & sgx_misc_reserved_mask ||
secs->attributes & sgx_attributes_reserved_mask ||
secs->xfrm & sgx_xfrm_reserved_mask)
return -EINVAL;

if (secs->size >= max_size)
return -EINVAL;

/Jarkko

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Jarkko Sakkinen]

On Mon, Jun 29, 2020 at 08:27:19AM -0700, Sean Christopherson wrote:
> On Sat, Jun 27, 2020 at 07:43:35PM +0200, Borislav Petkov wrote:
> > On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> > > +static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct 
> > > *sigstruct,
> > > +  void *token)
> > > +{
> > > + u64 mrsigner[4];
> > > + int ret;
> > > + int i;
> > > + int j;
> > > +
> > > + /* Check that the required attributes have been authorized. */
> > > + if (encl->secs_attributes & ~encl->allowed_attributes)
> > > + return -EACCES;
> > > +
> > > + ret = sgx_get_key_hash(sigstruct->modulus, mrsigner);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + mutex_lock(>lock);
> > > +
> > > + if (atomic_read(>flags) & SGX_ENCL_INITIALIZED) {
> > > + ret = -EFAULT;
> > > + goto err_out;
> > > + }
> > 
> > That test should be the first thing this function or its caller does.
> 
> Hmm, I was going to say that SGX_ENCL_INITIALIZED can't be checked until
> encl->lock is held, but that's not true for this path as mutual exclusion
> is provided by the SGX_ENCL_IOCTL flag.  So yeah, this can be checked at
> the same time as SGX_ENCL_CREATED in sgx_ioc_enclave_init().
> 
> > > + for (i = 0; i < SGX_EINIT_SLEEP_COUNT; i++) {
> > > + for (j = 0; j < SGX_EINIT_SPIN_COUNT; j++) {
> > 
> > Ew, what's that double-loop for?
> > 
> > It tries to init an enclave a bunch of times. Why does it need to init
> > more than once?
> 
> ENCLS[EINIT] is interruptible because it has such a high latency, e.g. 50k+
> cycles on success.  If an IRQ/NMI/SMI becomes pending, EINIT may fail with
> SGX_UNMASKED_EVENT so that the event can be serviced.
> 
> The idea behind the double loop is to try EINIT in a tight loop, then back
> off and sleep for a while before retrying that tight inner loop.
> 
> > > + ret = sgx_einit(sigstruct, token, encl->secs.epc_page,
> > > + mrsigner);
> > > + if (ret == SGX_UNMASKED_EVENT)
> > > + continue;
> > > + else
> > > + break;
> > > + }
> > > +
> > > + if (ret != SGX_UNMASKED_EVENT)
> > > + break;
> > > +
> > > + msleep_interruptible(SGX_EINIT_SLEEP_TIME);
> > > +
> > > + if (signal_pending(current)) {
> > > + ret = -ERESTARTSYS;
> > > + goto err_out;
> > > + }
> > > + }
> > > +
> > > + if (ret & ENCLS_FAULT_FLAG) {
> > > + if (encls_failed(ret))
> > > + ENCLS_WARN(ret, "EINIT");
> > > +
> > > + sgx_encl_destroy(encl);
> > > + ret = -EFAULT;
> > > + } else if (ret) {
> > > + pr_debug("EINIT returned %d\n", ret);
> > > + ret = -EPERM;
> > > + } else {
> > > + atomic_or(SGX_ENCL_INITIALIZED, >flags);
> > > + }
> > > +
> > > +err_out:
> > > + mutex_unlock(>lock);
> > > + return ret;
> > > +}
> > > +
> > > +/**
> > > + * sgx_ioc_enclave_init - handler for %SGX_IOC_ENCLAVE_INIT
> > > + *
> > > + * @filep:   open file to /dev/sgx
> > 
> > @encl:   pointer to an enclave instance (via ioctl() file pointer)
> > 
> > > + * @arg: userspace pointer to a struct sgx_enclave_init instance
> > > + *
> > > + * Flush any outstanding enqueued EADD operations and perform EINIT.  The
> > > + * Launch Enclave Public Key Hash MSRs are rewritten as necessary to 
> > > match
> > > + * the enclave's MRSIGNER, which is caculated from the provided 
> > > sigstruct.
> > > + *
> > > + * Return:
> > > + *   0 on success,
> > > + *   SGX error code on EINIT failure,
> > > + *   -errno otherwise
> > > + */
> > > +static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg)
> > > +{
> > > + struct sgx_sigstruct *sigstruct;
> > > + struct sgx_enclave_init einit;
> > > + struct page *initp_page;
> > > + void *token;
> > > + int ret;
> > > +
> > > + if (!(atomic_read(>flags) & SGX_ENCL_CREATED))
> > 
> > Might just as well check the other flags: doing EINIT on an already
> > initialized enclave - SGX_ENCL_INITIALIZED - is perhaps a nono or
> > similarly on a SGX_ENCL_DEAD enclave.
> > 
> > And you could do similar sanity checks in the other ioctl functions.
> 
> Ya, as above, SGX_ENCL_INITIALIZED can be checked here.
> 
> SGX_ENCL_DEAD is actually already checked in in the top level sgx_ioctl(),
> i.e. the check in sgx_encl_add_page() can technically be flat out dropped.
> 
> I say "technically" because I'm a bit torn over SGX_ENCL_DEAD; encl->lock
> must be held to SGX_ENCL_DEAD (the page fault and reclaim flows rely on
> this), but as it stands today only ioctl() paths (guarded by SGX_ENCL_IOCTL)
> and sgx_release() (makes the ioctls() unreachable) set SGX_ENCL_DEAD.
> 
> So it's safe to check SGX_ENCL_DEAD from ioctl() context without holding
> encl->lock, at least in the current code base, but it feels weird/sketchy.
> 
> In the end I don't think I have a strong 

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Jarkko Sakkinen]

On Sat, Jun 27, 2020 at 07:43:35PM +0200, Borislav Petkov wrote:
> On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> > +static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct 
> > *sigstruct,
> > +void *token)
> > +{
> > +   u64 mrsigner[4];
> > +   int ret;
> > +   int i;
> > +   int j;
> > +
> > +   /* Check that the required attributes have been authorized. */
> > +   if (encl->secs_attributes & ~encl->allowed_attributes)
> > +   return -EACCES;
> > +
> > +   ret = sgx_get_key_hash(sigstruct->modulus, mrsigner);
> > +   if (ret)
> > +   return ret;
> > +
> > +   mutex_lock(>lock);
> > +
> > +   if (atomic_read(>flags) & SGX_ENCL_INITIALIZED) {
> > +   ret = -EFAULT;
> > +   goto err_out;
> > +   }
> 
> That test should be the first thing this function or its caller does.

Fixed.

> 
> > +   for (i = 0; i < SGX_EINIT_SLEEP_COUNT; i++) {
> > +   for (j = 0; j < SGX_EINIT_SPIN_COUNT; j++) {
> 
> Ew, what's that double-loop for?
> 
> It tries to init an enclave a bunch of times. Why does it need to init
> more than once?

>From SDM:

"Periodically, EINIT polls for certain asynchronous events. If such an
event is detected, it completes with failure code (ZF=1 and RAX =
SGX_UNMASKED_EVENT), and RIP is incremented to point to the next
instruction. These events includes external interrupts, non-maskable
interrupts, system-management interrupts, machine checks, INIT signals,
and the VMX-preemption timer. EINIT does not fail if the pending event
is inhibited (e.g., external interrupts could be inhibited due to
blocking by MOV SS blocking or by STI)."

Not exactly sure though why this must be polled inside the kernel though.

> 
> > +   ret = sgx_einit(sigstruct, token, encl->secs.epc_page,
> > +   mrsigner);
> > +   if (ret == SGX_UNMASKED_EVENT)
> > +   continue;
> > +   else
> > +   break;
> > +   }
> > +
> > +   if (ret != SGX_UNMASKED_EVENT)
> > +   break;
> > +
> > +   msleep_interruptible(SGX_EINIT_SLEEP_TIME);
> > +
> > +   if (signal_pending(current)) {
> > +   ret = -ERESTARTSYS;
> > +   goto err_out;
> > +   }
> > +   }
> > +
> > +   if (ret & ENCLS_FAULT_FLAG) {
> > +   if (encls_failed(ret))
> > +   ENCLS_WARN(ret, "EINIT");
> > +
> > +   sgx_encl_destroy(encl);
> > +   ret = -EFAULT;
> > +   } else if (ret) {
> > +   pr_debug("EINIT returned %d\n", ret);
> > +   ret = -EPERM;
> > +   } else {
> > +   atomic_or(SGX_ENCL_INITIALIZED, >flags);
> > +   }
> > +
> > +err_out:
> > +   mutex_unlock(>lock);
> > +   return ret;
> > +}
> > +
> > +/**
> > + * sgx_ioc_enclave_init - handler for %SGX_IOC_ENCLAVE_INIT
> > + *
> > + * @filep: open file to /dev/sgx
> 
> @encl:   pointer to an enclave instance (via ioctl() file pointer)
> 
> > + * @arg:   userspace pointer to a struct sgx_enclave_init instance
> > + *
> > + * Flush any outstanding enqueued EADD operations and perform EINIT.  The
> > + * Launch Enclave Public Key Hash MSRs are rewritten as necessary to match
> > + * the enclave's MRSIGNER, which is caculated from the provided sigstruct.
> > + *
> > + * Return:
> > + *   0 on success,
> > + *   SGX error code on EINIT failure,
> > + *   -errno otherwise
> > + */
> > +static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg)
> > +{
> > +   struct sgx_sigstruct *sigstruct;
> > +   struct sgx_enclave_init einit;
> > +   struct page *initp_page;
> > +   void *token;
> > +   int ret;
> > +
> > +   if (!(atomic_read(>flags) & SGX_ENCL_CREATED))
> 
> Might just as well check the other flags: doing EINIT on an already
> initialized enclave - SGX_ENCL_INITIALIZED - is perhaps a nono or
> similarly on a SGX_ENCL_DEAD enclave.
> 
> And you could do similar sanity checks in the other ioctl functions.

Agreed (see my earlier response, let's continue this discussion there).

/Jarkko

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Jarkko Sakkinen]

On Fri, Jun 26, 2020 at 05:34:00PM +0200, Borislav Petkov wrote:
> On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> 
> ...
> 
> This could use some commenting along the lines of:
> 
> "— If the enclave developer requires measurement of the page as a
> proof for the content, use EEXTEND to add a measurement for 256 bytes of
> the page. Repeat this operation until the entire page is measured."
> 
> At least this text from the SDM maps to the 256 bytes below. Otherwise
> it is magic.

Copied with pride:

/*
 * If the caller requires measurement of the page as a proof for the content,
 * use EEXTEND to add a measurement for 256 bytes of the page. Repeat this
 * operation until the entire page is measured."
 */

> > +static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
> > +unsigned long offset, unsigned long length,
> > +struct sgx_secinfo *secinfo, unsigned long flags)
> > +{
> > +   struct sgx_encl_page *encl_page;
> > +   struct sgx_epc_page *epc_page;
> > +   int ret;
> > +
> > +   encl_page = sgx_encl_page_alloc(encl, offset, secinfo->flags);
> > +   if (IS_ERR(encl_page))
> > +   return PTR_ERR(encl_page);
> > +
> > +   epc_page = __sgx_alloc_epc_page();
> > +   if (IS_ERR(epc_page)) {
> > +   kfree(encl_page);
> > +   return PTR_ERR(epc_page);
> > +   }
> > +
> > +   if (atomic_read(>flags) &
> > +   (SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD)) {
> > +   ret = -EFAULT;
> > +   goto err_out_free;
> > +   }
> 
> You can do this first thing when you enter the function so that
> you don't have to allocate needlessly in the error case, when
> SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD is set.

Updated version:

static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
 unsigned long offset, unsigned long length,
 struct sgx_secinfo *secinfo, unsigned long flags)
{
struct sgx_encl_page *encl_page;
struct sgx_epc_page *epc_page;
struct sgx_va_page *va_page;
int ret;

if (atomic_read(>flags) & SGX_ENCL_INITIALIZED)
return -EFAULT;

SGX_ENCL_DEAD check is unnecessary altogether as this flag cannot be
possibly be unset inside ioctl. 'sgx_release()' will set it which is
the release callback for the enclave file.

'sgx_ioctl()' also unnecessarily has this check I just noticed (and
removed).

> "uninitialized"?
> 
> Where is the test for SGX_ENCL_INITIALIZED and erroring out otherwise?
> 
> I.e., what happens if you add pages to an initialized enclave?

Because of historical reasons it is in sgx_encl_add_page(). Then we
allowed ioctl's operate on enclave concurrently. Today we enforce
sequential operation on a single enclave with SGX_ENCL_IOCTL flag
because that is the only sane way to use the construction operations.

Therefore the check can be moved to sgx_ioc_encl_add_pages() if you
request so but first I have one remark to discuss.

I noticed that sometimes wrong state flags turn into -EINVAL and
sometimes into -EFAULT (like in the previous case). I'd suggest
that when the ioctl is blocked based encl->flags and only on that,
the ioctl would return -ENOIOCTLCMD in both cases, i.e. this
command is not available.

That would give much better aids for debugging user space code.

> 
> > + * measurement with the contents of the page. The address range of pages 
> > must
> > + * be contiguous.
> 
> Must? Who is enforcing this? I'm trying to find where...

Unfortunately I cannot recall what I meant when I wrote that. I removed
that sentence. I'm not sure what I meant exactly when I used 'contiguous'
here.

> > The SECINFO and measurement mask are applied to all pages.
> > + *
> > + * A SECINFO for a TCS is required to always contain zero permissions 
> > because
> > + * CPU silently zeros them. Allowing anything else would cause a mismatch 
> > in
> > + * the measurement.
> > + *
> > + * mmap()'s protection bits are capped by the page permissions. For each 
> > page
> > + * address, the maximum protection bits are computed with the following
> > + * heuristics:
> > + *
> > + * 1. A regular page: PROT_R, PROT_W and PROT_X match the SECINFO 
> > permissions.
> > + * 2. A TCS page: PROT_R | PROT_W.
> > + *
> > + * mmap() is not allowed to surpass the minimum of the maximum protection 
> > bits
> > + * within the given address range.
> > + *
> > + * If ENCLS opcode fails, that effectively means that EPC has been 
> > invalidated.
> > + * When this happens the enclave is destroyed and -EIO is returned to the
> > + * caller.
> > + *
> > + * Return:
> > + *   0 on success,
> > + *   -EACCES if an executable source page is located in a noexec partition,
> > + *   -EIO if either ENCLS[EADD] or ENCLS[EEXTEND] fails
> > + *   -errno otherwise
> > + */
> > +static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user 
> > *arg)
> > +{
> > +   struct sgx_enclave_add_pages addp;
> > 

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Jarkko Sakkinen]

On Fri, Jun 26, 2020 at 04:20:19PM +0200, Borislav Petkov wrote:
> On Fri, Jun 26, 2020 at 07:16:27AM -0700, Sean Christopherson wrote:
> > That being said, I agree that it would be safer to move 
> > sgx_calc_ssaframesize()
> > inside sgx_validate_secs() and only compute encl_size after the secs is
> > validated.
> 

Changed as

if (!secs->ssa_frame_size)
return -EINVAL;

if (sgx_calc_ssa_frame_size(secs->miscselect, secs->xfrm) >
secs->ssa_frame_size)
return -EINVAL;

/Jarkko

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Jarkko Sakkinen]

On Fri, Jun 26, 2020 at 11:14:19AM +0200, Borislav Petkov wrote:
> On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst 
> > b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > index 59472cd6a11d..35f713e3a267 100644
> > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > @@ -323,6 +323,7 @@ Code  Seq#Include File  
> >  Comments
> >   
> > 
> >  0xA3  90-9F  linux/dtlk.h
> >  0xA4  00-1F  uapi/linux/tee.h
> > Generic TEE subsystem
> > +0xA4  00-1F  uapi/asm/sgx.h  Intel 
> > SGX subsystem (a legit conflict as TEE and SGX do not co-exist)
> 
> Maybe add  ?
> 
> >  0xAA  00-3F  linux/uapi/linux/userfaultfd.h
> >  0xAB  00-1F  linux/nbd.h
> >  0xAC  00-1F  linux/raw.h
> 
> ...
> 
> > +static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
> > +{
> > +   unsigned long encl_size = secs->size + PAGE_SIZE;
> 
> Wait, you just copied @secs from user memory in sgx_ioc_enclave_create()
> and now use ->size unverified? You're kidding, right?

The validation is done in sgx_validate_secs().

> 
> > +   struct sgx_epc_page *secs_epc;
> > +   unsigned long ssaframesize;
> > +   struct sgx_pageinfo pginfo;
> > +   struct sgx_secinfo secinfo;
> > +   struct file *backing;
> > +   long ret;
> > +
> > +   if (atomic_read(>flags) & SGX_ENCL_CREATED)
> > +   return -EINVAL;
> > +
> > +   ssaframesize = sgx_calc_ssaframesize(secs->miscselect, secs->xfrm);
> 
> So this is using more un-validated user input to do further calculations.
> What can possibly go wrong?
> 
> I sure hope *I* am wrong and am missing something here.
> 
> If not, please, for the next version, audit all your user input and
> validate it before using it. Srsly.

It works but is unclean. I'd guess reason for this is just that code has
evolved into this state over time.

I'd just move the call to sgx_calc_ssaframesize() inside
sgx_validate_secs().

/Jarkko

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Sean Christopherson]

On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> +static int sgx_validate_secs(const struct sgx_secs *secs,
> +  unsigned long ssaframesize)
> +{
> + if (secs->size < (2 * PAGE_SIZE) || !is_power_of_2(secs->size))
> + return -EINVAL;
> +
> + if (secs->base & (secs->size - 1))
> + return -EINVAL;
> +
> + if (secs->miscselect & sgx_misc_reserved_mask ||
> + secs->attributes & sgx_attributes_reserved_mask ||
> + secs->xfrm & sgx_xfrm_reserved_mask)
> + return -EINVAL;
> +
> + if (secs->attributes & SGX_ATTR_MODE64BIT) {
> + if (secs->size > sgx_encl_size_max_64)
> + return -EINVAL;
> + } else if (secs->size > sgx_encl_size_max_32)
> + return -EINVAL;

These should be >=, not >, the SDM uses one of those fancy ≥ ligatures.

Internal versions use more obvious pseudocode, e.g.:

if ((DS:TMP_SECS.ATTRIBUTES.MODE64BIT = 1) AND
(DS:TMP_SECS.SIZE AND (~((1 << CPUID.18.0:EDX[15:8]) – 1)))
{
#GP(0);
}

> +
> + if (!(secs->xfrm & XFEATURE_MASK_FP) ||
> + !(secs->xfrm & XFEATURE_MASK_SSE) ||
> + (((secs->xfrm >> XFEATURE_BNDREGS) & 1) !=
> +  ((secs->xfrm >> XFEATURE_BNDCSR) & 1)))
> + return -EINVAL;
> +
> + if (!secs->ssa_frame_size || ssaframesize > secs->ssa_frame_size)
> + return -EINVAL;
> +
> + if (memchr_inv(secs->reserved1, 0, sizeof(secs->reserved1)) ||
> + memchr_inv(secs->reserved2, 0, sizeof(secs->reserved2)) ||
> + memchr_inv(secs->reserved3, 0, sizeof(secs->reserved3)) ||
> + memchr_inv(secs->reserved4, 0, sizeof(secs->reserved4)))
> + return -EINVAL;
> +
> + return 0;
> +}

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Borislav Petkov]

On Mon, Jun 29, 2020 at 08:27:19AM -0700, Sean Christopherson wrote:
> Hmm, I was going to say that SGX_ENCL_INITIALIZED can't be checked until
> encl->lock is held, but that's not true for this path as mutual exclusion
> is provided by the SGX_ENCL_IOCTL flag.  So yeah, this can be checked at
> the same time as SGX_ENCL_CREATED in sgx_ioc_enclave_init().

Right, so my point is to have state checks for flags which make sense in
all ioctl entry points, in order to catch a misuse early. But we're on
the same page.

> ENCLS[EINIT] is interruptible because it has such a high latency, e.g. 50k+
> cycles on success.  If an IRQ/NMI/SMI becomes pending, EINIT may fail with
> SGX_UNMASKED_EVENT so that the event can be serviced.
>
> The idea behind the double loop is to try EINIT in a tight loop, then back
> off and sleep for a while before retrying that tight inner loop.

That gist of that kinda wants to be in a comment over that double-loop for
future on-lookers.

Thx.

-- 
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Sean Christopherson]

On Sat, Jun 27, 2020 at 07:43:35PM +0200, Borislav Petkov wrote:
> On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> > +static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct 
> > *sigstruct,
> > +void *token)
> > +{
> > +   u64 mrsigner[4];
> > +   int ret;
> > +   int i;
> > +   int j;
> > +
> > +   /* Check that the required attributes have been authorized. */
> > +   if (encl->secs_attributes & ~encl->allowed_attributes)
> > +   return -EACCES;
> > +
> > +   ret = sgx_get_key_hash(sigstruct->modulus, mrsigner);
> > +   if (ret)
> > +   return ret;
> > +
> > +   mutex_lock(>lock);
> > +
> > +   if (atomic_read(>flags) & SGX_ENCL_INITIALIZED) {
> > +   ret = -EFAULT;
> > +   goto err_out;
> > +   }
> 
> That test should be the first thing this function or its caller does.

Hmm, I was going to say that SGX_ENCL_INITIALIZED can't be checked until
encl->lock is held, but that's not true for this path as mutual exclusion
is provided by the SGX_ENCL_IOCTL flag.  So yeah, this can be checked at
the same time as SGX_ENCL_CREATED in sgx_ioc_enclave_init().

> > +   for (i = 0; i < SGX_EINIT_SLEEP_COUNT; i++) {
> > +   for (j = 0; j < SGX_EINIT_SPIN_COUNT; j++) {
> 
> Ew, what's that double-loop for?
> 
> It tries to init an enclave a bunch of times. Why does it need to init
> more than once?

ENCLS[EINIT] is interruptible because it has such a high latency, e.g. 50k+
cycles on success.  If an IRQ/NMI/SMI becomes pending, EINIT may fail with
SGX_UNMASKED_EVENT so that the event can be serviced.

The idea behind the double loop is to try EINIT in a tight loop, then back
off and sleep for a while before retrying that tight inner loop.

> > +   ret = sgx_einit(sigstruct, token, encl->secs.epc_page,
> > +   mrsigner);
> > +   if (ret == SGX_UNMASKED_EVENT)
> > +   continue;
> > +   else
> > +   break;
> > +   }
> > +
> > +   if (ret != SGX_UNMASKED_EVENT)
> > +   break;
> > +
> > +   msleep_interruptible(SGX_EINIT_SLEEP_TIME);
> > +
> > +   if (signal_pending(current)) {
> > +   ret = -ERESTARTSYS;
> > +   goto err_out;
> > +   }
> > +   }
> > +
> > +   if (ret & ENCLS_FAULT_FLAG) {
> > +   if (encls_failed(ret))
> > +   ENCLS_WARN(ret, "EINIT");
> > +
> > +   sgx_encl_destroy(encl);
> > +   ret = -EFAULT;
> > +   } else if (ret) {
> > +   pr_debug("EINIT returned %d\n", ret);
> > +   ret = -EPERM;
> > +   } else {
> > +   atomic_or(SGX_ENCL_INITIALIZED, >flags);
> > +   }
> > +
> > +err_out:
> > +   mutex_unlock(>lock);
> > +   return ret;
> > +}
> > +
> > +/**
> > + * sgx_ioc_enclave_init - handler for %SGX_IOC_ENCLAVE_INIT
> > + *
> > + * @filep: open file to /dev/sgx
> 
> @encl:   pointer to an enclave instance (via ioctl() file pointer)
> 
> > + * @arg:   userspace pointer to a struct sgx_enclave_init instance
> > + *
> > + * Flush any outstanding enqueued EADD operations and perform EINIT.  The
> > + * Launch Enclave Public Key Hash MSRs are rewritten as necessary to match
> > + * the enclave's MRSIGNER, which is caculated from the provided sigstruct.
> > + *
> > + * Return:
> > + *   0 on success,
> > + *   SGX error code on EINIT failure,
> > + *   -errno otherwise
> > + */
> > +static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg)
> > +{
> > +   struct sgx_sigstruct *sigstruct;
> > +   struct sgx_enclave_init einit;
> > +   struct page *initp_page;
> > +   void *token;
> > +   int ret;
> > +
> > +   if (!(atomic_read(>flags) & SGX_ENCL_CREATED))
> 
> Might just as well check the other flags: doing EINIT on an already
> initialized enclave - SGX_ENCL_INITIALIZED - is perhaps a nono or
> similarly on a SGX_ENCL_DEAD enclave.
> 
> And you could do similar sanity checks in the other ioctl functions.

Ya, as above, SGX_ENCL_INITIALIZED can be checked here.

SGX_ENCL_DEAD is actually already checked in in the top level sgx_ioctl(),
i.e. the check in sgx_encl_add_page() can technically be flat out dropped.

I say "technically" because I'm a bit torn over SGX_ENCL_DEAD; encl->lock
must be held to SGX_ENCL_DEAD (the page fault and reclaim flows rely on
this), but as it stands today only ioctl() paths (guarded by SGX_ENCL_IOCTL)
and sgx_release() (makes the ioctls() unreachable) set SGX_ENCL_DEAD.

So it's safe to check SGX_ENCL_DEAD from ioctl() context without holding
encl->lock, at least in the current code base, but it feels weird/sketchy.

In the end I don't think I have a strong opinion.  Removing the technically
unnecessary DEAD check in sgx_encl_add_page() is the simplest change, so it
may make sense to do that and nothing more for initial upstreaming.  Long
term, I fully expect we'll add paths that set SGX_ENCL_DEAD 

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Borislav Petkov]

On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> +static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct 
> *sigstruct,
> +  void *token)
> +{
> + u64 mrsigner[4];
> + int ret;
> + int i;
> + int j;
> +
> + /* Check that the required attributes have been authorized. */
> + if (encl->secs_attributes & ~encl->allowed_attributes)
> + return -EACCES;
> +
> + ret = sgx_get_key_hash(sigstruct->modulus, mrsigner);
> + if (ret)
> + return ret;
> +
> + mutex_lock(>lock);
> +
> + if (atomic_read(>flags) & SGX_ENCL_INITIALIZED) {
> + ret = -EFAULT;
> + goto err_out;
> + }

That test should be the first thing this function or its caller does.

> + for (i = 0; i < SGX_EINIT_SLEEP_COUNT; i++) {
> + for (j = 0; j < SGX_EINIT_SPIN_COUNT; j++) {

Ew, what's that double-loop for?

It tries to init an enclave a bunch of times. Why does it need to init
more than once?

> + ret = sgx_einit(sigstruct, token, encl->secs.epc_page,
> + mrsigner);
> + if (ret == SGX_UNMASKED_EVENT)
> + continue;
> + else
> + break;
> + }
> +
> + if (ret != SGX_UNMASKED_EVENT)
> + break;
> +
> + msleep_interruptible(SGX_EINIT_SLEEP_TIME);
> +
> + if (signal_pending(current)) {
> + ret = -ERESTARTSYS;
> + goto err_out;
> + }
> + }
> +
> + if (ret & ENCLS_FAULT_FLAG) {
> + if (encls_failed(ret))
> + ENCLS_WARN(ret, "EINIT");
> +
> + sgx_encl_destroy(encl);
> + ret = -EFAULT;
> + } else if (ret) {
> + pr_debug("EINIT returned %d\n", ret);
> + ret = -EPERM;
> + } else {
> + atomic_or(SGX_ENCL_INITIALIZED, >flags);
> + }
> +
> +err_out:
> + mutex_unlock(>lock);
> + return ret;
> +}
> +
> +/**
> + * sgx_ioc_enclave_init - handler for %SGX_IOC_ENCLAVE_INIT
> + *
> + * @filep:   open file to /dev/sgx

@encl:   pointer to an enclave instance (via ioctl() file pointer)

> + * @arg: userspace pointer to a struct sgx_enclave_init instance
> + *
> + * Flush any outstanding enqueued EADD operations and perform EINIT.  The
> + * Launch Enclave Public Key Hash MSRs are rewritten as necessary to match
> + * the enclave's MRSIGNER, which is caculated from the provided sigstruct.
> + *
> + * Return:
> + *   0 on success,
> + *   SGX error code on EINIT failure,
> + *   -errno otherwise
> + */
> +static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg)
> +{
> + struct sgx_sigstruct *sigstruct;
> + struct sgx_enclave_init einit;
> + struct page *initp_page;
> + void *token;
> + int ret;
> +
> + if (!(atomic_read(>flags) & SGX_ENCL_CREATED))

Might just as well check the other flags: doing EINIT on an already
initialized enclave - SGX_ENCL_INITIALIZED - is perhaps a nono or
similarly on a SGX_ENCL_DEAD enclave.

And you could do similar sanity checks in the other ioctl functions.

-- 
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Borislav Petkov]

On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:

...

This could use some commenting along the lines of:

"— If the enclave developer requires measurement of the page as a
proof for the content, use EEXTEND to add a measurement for 256 bytes of
the page. Repeat this operation until the entire page is measured."

At least this text from the SDM maps to the 256 bytes below. Otherwise
it is magic.

> +static int __sgx_encl_extend(struct sgx_encl *encl,
> +  struct sgx_epc_page *epc_page)
> +{
> + int ret;
> + int i;
> +
> + for (i = 0; i < 16; i++) {
> + ret = __eextend(sgx_get_epc_addr(encl->secs.epc_page),
> + sgx_get_epc_addr(epc_page) + (i * 0x100));
> + if (ret) {
> + if (encls_failed(ret))
> + ENCLS_WARN(ret, "EEXTEND");
> + return -EIO;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
> +  unsigned long offset, unsigned long length,
> +  struct sgx_secinfo *secinfo, unsigned long flags)
> +{
> + struct sgx_encl_page *encl_page;
> + struct sgx_epc_page *epc_page;
> + int ret;
> +
> + encl_page = sgx_encl_page_alloc(encl, offset, secinfo->flags);
> + if (IS_ERR(encl_page))
> + return PTR_ERR(encl_page);
> +
> + epc_page = __sgx_alloc_epc_page();
> + if (IS_ERR(epc_page)) {
> + kfree(encl_page);
> + return PTR_ERR(epc_page);
> + }
> +
> + if (atomic_read(>flags) &
> + (SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD)) {
> + ret = -EFAULT;
> + goto err_out_free;
> + }

You can do this first thing when you enter the function so that
you don't have to allocate needlessly in the error case, when
SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD is set.

> +
> + mmap_read_lock(current->mm);
> + mutex_lock(>lock);
> +
> + /*
> +  * Insert prior to EADD in case of OOM.  EADD modifies MRENCLAVE, i.e.
> +  * can't be gracefully unwound, while failure on EADD/EXTEND is limited
> +  * to userspace errors (or kernel/hardware bugs).
> +  */
> + ret = radix_tree_insert(>page_tree, PFN_DOWN(encl_page->desc),
> + encl_page);
> + if (ret)
> + goto err_out_unlock;
> +
> + ret = __sgx_encl_add_page(encl, encl_page, epc_page, secinfo,
> +   src);
> + if (ret)
> + goto err_out;
> +
> + /*
> +  * Complete the "add" before doing the "extend" so that the "add"
> +  * isn't in a half-baked state in the extremely unlikely scenario the
> +  * the enclave will be destroyed in response to EEXTEND failure.
> +  */
> + encl_page->encl = encl;
> + encl_page->epc_page = epc_page;
> + encl->secs_child_cnt++;
> +
> + if (flags & SGX_PAGE_MEASURE) {
> + ret = __sgx_encl_extend(encl, epc_page);
> + if (ret)
> + goto err_out;
> + }
> +
> + mutex_unlock(>lock);
> + mmap_read_unlock(current->mm);
> + return ret;
> +
> +err_out:
> + radix_tree_delete(_page->encl->page_tree,
> +   PFN_DOWN(encl_page->desc));
> +
> +err_out_unlock:
> + mutex_unlock(>lock);
> + mmap_read_unlock(current->mm);
> +
> +err_out_free:
> + sgx_free_epc_page(epc_page);
> + kfree(encl_page);
> +
> + /*
> +  * Destroy enclave on ENCLS failure as this means that EPC has been
> +  * invalidated.
> +  */
> + if (ret == -EIO) {
> + mutex_lock(>lock);
> + sgx_encl_destroy(encl);
> + mutex_unlock(>lock);
> + }
> +
> + return ret;
> +}
> +
> +/**
> + * sgx_ioc_enclave_add_pages() - The handler for %SGX_IOC_ENCLAVE_ADD_PAGES
> + * @encl:   pointer to an enclave instance (via ioctl() file pointer)
> + * @arg: a user pointer to a struct sgx_enclave_add_pages instance
> + *
> + * Add one or more pages to an uninitialized enclave, and optionally extend 
> the

"uninitialized"?

Where is the test for SGX_ENCL_INITIALIZED and erroring out otherwise?

I.e., what happens if you add pages to an initialized enclave?

> + * measurement with the contents of the page. The address range of pages must
> + * be contiguous.

Must? Who is enforcing this? I'm trying to find where...

> The SECINFO and measurement mask are applied to all pages.
> + *
> + * A SECINFO for a TCS is required to always contain zero permissions because
> + * CPU silently zeros them. Allowing anything else would cause a mismatch in
> + * the measurement.
> + *
> + * mmap()'s protection bits are capped by the page permissions. For each page
> + * address, the maximum protection bits are computed with the following
> + * heuristics:
> + *
> + * 1. A regular page: PROT_R, PROT_W and PROT_X match the SECINFO 
> 

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Borislav Petkov]

On Fri, Jun 26, 2020 at 07:16:27AM -0700, Sean Christopherson wrote:
> That being said, I agree that it would be safer to move 
> sgx_calc_ssaframesize()
> inside sgx_validate_secs() and only compute encl_size after the secs is
> validated.

Yap, that would be the right thing to do.

-- 
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Jarkko Sakkinen]

On Thu, Jun 25, 2020 at 11:34:48AM -0700, Sean Christopherson wrote:
> On Thu, Jun 25, 2020 at 07:23:19PM +0200, Borislav Petkov wrote:
> > Also, you had all patches until now split nice and logically doing one
> > thing only.
> > 
> > But this one is huge. Why?
> > 
> > Why can't you split out the facilities which the driver uses: encl.[ch]
> > into a patch, then ioctl.c into a separate one and then the driver into
> > a third one? Or do they all belong together inseparably?
> > 
> > I guess I'll find out eventually but it would've been nice if they were
> > split out...
> 
> Hmm, I think the most reasonable way to break up this beast would be to
> incrementally introduce functionality.  E.g. four or so patches, one for
> each ioctl() of ENCLAVE_CREATE, ENCLAVE_ADD_PAGES, ENCLAVE_INIT and
> ENCLAVE_SET_ATTRIBUTE, in that order.
> 
> Splitting up by file probably wouldn't work very well.  The split is
> pretty arbitrary, e.g. encl.[ch] isn't simply a pure representation of an
> enclave, there is a lot of the driver details/dependencies in there, i.e.
> the functionality between encl/ioctl/driver is all pretty intertwined.
> 
> But I think serially introducing each ioctl() would be fairly clean, and
> would help readers/reviewers better understand SGX as the patches would
> naturally document the process of building an enclave, e.g. CREATE the
> enclave, then ADD_PAGES, then INIT the enclave.  SET_ATTRIBUTE is a bit
> of an outlier in that it would be chronologically out of order with
> respect to building the enclave, but I think that's ok. 
> 
> Jarkko, thoughts?

I proposed the same before I go this email so I guess we have a
consensus here.

/Jarkko

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Jarkko Sakkinen]

On Thu, Jun 25, 2020 at 08:53:34PM +0200, Borislav Petkov wrote:
> On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> 
> > Subject: Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver
>^
>Add

I'll change it to "Add SGX enclave driver".

> 
> > Intel Software Guard eXtensions (SGX) is a set of CPU instructions that
> > can be used by applications to set aside private regions of code and
> > data. The code outside the SGX hosted software entity is disallowed to
> > access the memory inside the enclave enforced by the CPU. We call these
> > entities as enclaves.
> 
> s/as //
> 
> > This commit implements a driver that provides an ioctl API to construct
> 
> s/This commit implements/Implement/
> 
> > and run enclaves. Enclaves are constructed from pages residing in
> > reserved physical memory areas. The contents of these pages can only be
> > accessed when they are mapped as part of an enclave, by a hardware
> > thread running inside the enclave.
> > 
> > The starting state of an enclave consists of a fixed measured set of
> > pages that are copied to the EPC during the construction process by
> > using ENCLS leaf functions and Software Enclave Control Structure (SECS)
> > that defines the enclave properties.
> > 
> > Enclave are constructed by using ENCLS leaf functions ECREATE, EADD and
> 
> Enclaves
> 
> > EINIT. ECREATE initializes SECS, EADD copies pages from system memory to
> > the EPC and EINIT check a given signed measurement and moves the enclave
> 
>  checks
> 
> > into a state ready for execution.
> > 
> > An initialized enclave can only be accessed through special Thread Control
> > Structure (TCS) pages by using ENCLU (ring-3 only) leaf EENTER.  This leaf
> > function converts a thread into enclave mode and continues the execution in
> > the offset defined by the TCS provided to EENTER. An enclave is exited
> > through syscall, exception, interrupts or by explicitly calling another
> > ENCLU leaf EEXIT.
> > 
> > The permissions, which enclave page is added will set the limit for maximum
> > permissions that can be set for mmap() and mprotect().
> 
> I can't parse that sentence.

Neither can I.

> > This will
> > effectively allow to build different security schemes between producers and
> > consumers of enclaves. Later on we can increase granularity with LSM hooks
> > for page addition (i.e. for producers) and mapping of the enclave (i.e. for
> > consumers)

I rephrased the whole paragraph:

"
The mmap() permissions are capped by the contained enclave page
permissions. The mapped areas must also be opaque, i.e. each page address
must contain a page. This logic is implemented in sgx_encl_may_map().
"

> Other than that, nice explanation. I like that in a commit message.
> 
> Thx.

Thank you.

> 
> -- 
> Regards/Gruss,
> Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

/Jarkko

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Sean Christopherson]

On Fri, Jun 26, 2020 at 11:14:19AM +0200, Borislav Petkov wrote:
> On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> > +static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
> > +{
> > +   unsigned long encl_size = secs->size + PAGE_SIZE;
> 
> Wait, you just copied @secs from user memory in sgx_ioc_enclave_create()
> and now use ->size unverified? You're kidding, right?

The size of the enclave is checked in sgx_validate_secs() before it is used
to configure the shmem backing.
 
> > +   struct sgx_epc_page *secs_epc;
> > +   unsigned long ssaframesize;
> > +   struct sgx_pageinfo pginfo;
> > +   struct sgx_secinfo secinfo;
> > +   struct file *backing;
> > +   long ret;
> > +
> > +   if (atomic_read(>flags) & SGX_ENCL_CREATED)
> > +   return -EINVAL;
> > +
> > +   ssaframesize = sgx_calc_ssaframesize(secs->miscselect, secs->xfrm);
> 
> So this is using more un-validated user input to do further calculations.
> What can possibly go wrong?

ssaframesize is also validated below, and the computations on miscselect and
xfm in sgx_calc_ssaframesize() are bounded such that bad input won't send
the kernel into the weeds.

That being said, I agree that it would be safer to move sgx_calc_ssaframesize()
inside sgx_validate_secs() and only compute encl_size after the secs is
validated.

> I sure hope *I* am wrong and am missing something here.
> 
> If not, please, for the next version, audit all your user input and
> validate it before using it. Srsly.
> 
> > +   if (sgx_validate_secs(secs, ssaframesize)) {
> > +   pr_debug("invalid SECS\n");
> > +   return -EINVAL;
> > +   }
> > +
> > +   backing = shmem_file_setup("SGX backing", encl_size + (encl_size >> 5),
> > +  VM_NORESERVE);
> > +   if (IS_ERR(backing))
> > +   return PTR_ERR(backing);
> > +
> > +   encl->backing = backing;
> > +
> > +   secs_epc = __sgx_alloc_epc_page();
> > +   if (IS_ERR(secs_epc)) {
> > +   ret = PTR_ERR(secs_epc);
> > +   goto err_out_backing;
> > +   }
> > +
> > +   encl->secs.epc_page = secs_epc;
> > +
> > +   pginfo.addr = 0;
> > +   pginfo.contents = (unsigned long)secs;
> > +   pginfo.metadata = (unsigned long)
> > +   pginfo.secs = 0;
> > +   memset(, 0, sizeof(secinfo));
> > +
> > +   ret = __ecreate((void *), sgx_get_epc_addr(secs_epc));
> > +   if (ret) {
> > +   pr_debug("ECREATE returned %ld\n", ret);
> > +   goto err_out;
> > +   }
> > +
> > +   if (secs->attributes & SGX_ATTR_DEBUG)
> > +   atomic_or(SGX_ENCL_DEBUG, >flags);
> > +
> > +   encl->secs.encl = encl;
> > +   encl->secs_attributes = secs->attributes;
> > +   encl->allowed_attributes |= SGX_ATTR_ALLOWED_MASK;
> > +   encl->base = secs->base;
> > +   encl->size = secs->size;
> > +   encl->ssaframesize = secs->ssa_frame_size;
> > +
> > +   /*
> > +* Set SGX_ENCL_CREATED only after the enclave is fully prepped.  This
> > +* allows setting and checking enclave creation without having to take
> > +* encl->lock.
> > +*/
> > +   atomic_or(SGX_ENCL_CREATED, >flags);
> > +
> > +   return 0;
> > +
> > +err_out:
> > +   sgx_free_epc_page(encl->secs.epc_page);
> > +   encl->secs.epc_page = NULL;
> > +
> > +err_out_backing:
> > +   fput(encl->backing);
> > +   encl->backing = NULL;
> > +
> > +   return ret;
> > +}
> > +
> > +/**
> > + * sgx_ioc_enclave_create - handler for %SGX_IOC_ENCLAVE_CREATE
> > + * @filep: open file to /dev/sgx
> 
> That's
> 
> @encl: enclave pointer
> 
> or so.
> 
> > + * @arg:   userspace pointer to a struct sgx_enclave_create instance
> > + *
> > + * Allocate kernel data structures for a new enclave and execute ECREATE 
> > after
> > + * verifying the correctness of the provided SECS.
> > + *
> > + * Note, enforcement of restricted and disallowed attributes is deferred 
> > until
> > + * sgx_ioc_enclave_init(), only the architectural correctness of the SECS 
> > is
> > + * checked by sgx_ioc_enclave_create().
> 
> Well, I don't see that checking. Where is it?
> 
> > + *
> > + * Return:
> > + *   0 on success,
> > + *   -errno otherwise
> > + */
> > +static long sgx_ioc_enclave_create(struct sgx_encl *encl, void __user *arg)
> > +{
> > +   struct sgx_enclave_create ecreate;
> > +   struct page *secs_page;
> > +   struct sgx_secs *secs;
> > +   int ret;
> > +
> > +   if (copy_from_user(, arg, sizeof(ecreate)))
> > +   return -EFAULT;
> > +
> > +   secs_page = alloc_page(GFP_KERNEL);
> > +   if (!secs_page)
> > +   return -ENOMEM;
> > +
> > +   secs = kmap(secs_page);
> > +   if (copy_from_user(secs, (void __user *)ecreate.src, sizeof(*secs))) {
> > +   ret = -EFAULT;
> > +   goto out;
> > +   }
> > +
> > +   ret = sgx_encl_create(encl, secs);
> > +
> > +out:
> > +   kunmap(secs_page);
> > +   __free_page(secs_page);
> > +   return ret;
> > +}
> 
> -- 
> Regards/Gruss,
> Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Jarkko Sakkinen]

On Thu, Jun 25, 2020 at 10:25:39PM +0200, Borislav Petkov wrote:
> On Thu, Jun 25, 2020 at 11:21:48PM +0300, Jarkko Sakkinen wrote:
> > Would be probably easier to review also this way because the commit kind
> > of rationalizes why things exist.
> > 
> > What do you think?
> 
> Sounds like a plan but you can do this for the next version - no need to
> do it now. I'll try to review this way, per ioctl as I said in my mail
> to Sean.

OK, sure I won't rush with a new version :-)

The code has been reworked so many times that it is somewhat easy to
make such split and I think it is one measure that an implementation is
somewhat sound when parts of functionality build up cleanly on top of
each other.

/Jarkko

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Borislav Petkov]

On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst 
> b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 59472cd6a11d..35f713e3a267 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -323,6 +323,7 @@ Code  Seq#Include File
>Comments
>   
> 
>  0xA3  90-9F  linux/dtlk.h
>  0xA4  00-1F  uapi/linux/tee.hGeneric 
> TEE subsystem
> +0xA4  00-1F  uapi/asm/sgx.h  Intel 
> SGX subsystem (a legit conflict as TEE and SGX do not co-exist)

Maybe add  ?

>  0xAA  00-3F  linux/uapi/linux/userfaultfd.h
>  0xAB  00-1F  linux/nbd.h
>  0xAC  00-1F  linux/raw.h

...

> +static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
> +{
> + unsigned long encl_size = secs->size + PAGE_SIZE;

Wait, you just copied @secs from user memory in sgx_ioc_enclave_create()
and now use ->size unverified? You're kidding, right?

> + struct sgx_epc_page *secs_epc;
> + unsigned long ssaframesize;
> + struct sgx_pageinfo pginfo;
> + struct sgx_secinfo secinfo;
> + struct file *backing;
> + long ret;
> +
> + if (atomic_read(>flags) & SGX_ENCL_CREATED)
> + return -EINVAL;
> +
> + ssaframesize = sgx_calc_ssaframesize(secs->miscselect, secs->xfrm);

So this is using more un-validated user input to do further calculations.
What can possibly go wrong?

I sure hope *I* am wrong and am missing something here.

If not, please, for the next version, audit all your user input and
validate it before using it. Srsly.

> + if (sgx_validate_secs(secs, ssaframesize)) {
> + pr_debug("invalid SECS\n");
> + return -EINVAL;
> + }
> +
> + backing = shmem_file_setup("SGX backing", encl_size + (encl_size >> 5),
> +VM_NORESERVE);
> + if (IS_ERR(backing))
> + return PTR_ERR(backing);
> +
> + encl->backing = backing;
> +
> + secs_epc = __sgx_alloc_epc_page();
> + if (IS_ERR(secs_epc)) {
> + ret = PTR_ERR(secs_epc);
> + goto err_out_backing;
> + }
> +
> + encl->secs.epc_page = secs_epc;
> +
> + pginfo.addr = 0;
> + pginfo.contents = (unsigned long)secs;
> + pginfo.metadata = (unsigned long)
> + pginfo.secs = 0;
> + memset(, 0, sizeof(secinfo));
> +
> + ret = __ecreate((void *), sgx_get_epc_addr(secs_epc));
> + if (ret) {
> + pr_debug("ECREATE returned %ld\n", ret);
> + goto err_out;
> + }
> +
> + if (secs->attributes & SGX_ATTR_DEBUG)
> + atomic_or(SGX_ENCL_DEBUG, >flags);
> +
> + encl->secs.encl = encl;
> + encl->secs_attributes = secs->attributes;
> + encl->allowed_attributes |= SGX_ATTR_ALLOWED_MASK;
> + encl->base = secs->base;
> + encl->size = secs->size;
> + encl->ssaframesize = secs->ssa_frame_size;
> +
> + /*
> +  * Set SGX_ENCL_CREATED only after the enclave is fully prepped.  This
> +  * allows setting and checking enclave creation without having to take
> +  * encl->lock.
> +  */
> + atomic_or(SGX_ENCL_CREATED, >flags);
> +
> + return 0;
> +
> +err_out:
> + sgx_free_epc_page(encl->secs.epc_page);
> + encl->secs.epc_page = NULL;
> +
> +err_out_backing:
> + fput(encl->backing);
> + encl->backing = NULL;
> +
> + return ret;
> +}
> +
> +/**
> + * sgx_ioc_enclave_create - handler for %SGX_IOC_ENCLAVE_CREATE
> + * @filep:   open file to /dev/sgx

That's

@encl: enclave pointer

or so.

> + * @arg: userspace pointer to a struct sgx_enclave_create instance
> + *
> + * Allocate kernel data structures for a new enclave and execute ECREATE 
> after
> + * verifying the correctness of the provided SECS.
> + *
> + * Note, enforcement of restricted and disallowed attributes is deferred 
> until
> + * sgx_ioc_enclave_init(), only the architectural correctness of the SECS is
> + * checked by sgx_ioc_enclave_create().

Well, I don't see that checking. Where is it?

> + *
> + * Return:
> + *   0 on success,
> + *   -errno otherwise
> + */
> +static long sgx_ioc_enclave_create(struct sgx_encl *encl, void __user *arg)
> +{
> + struct sgx_enclave_create ecreate;
> + struct page *secs_page;
> + struct sgx_secs *secs;
> + int ret;
> +
> + if (copy_from_user(, arg, sizeof(ecreate)))
> + return -EFAULT;
> +
> + secs_page = alloc_page(GFP_KERNEL);
> + if (!secs_page)
> + return -ENOMEM;
> +
> + secs = kmap(secs_page);
> + if (copy_from_user(secs, (void __user *)ecreate.src, sizeof(*secs))) {
> + ret = -EFAULT;
> + goto out;
> + }
> +
> + ret 

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Borislav Petkov]

On Thu, Jun 25, 2020 at 11:21:48PM +0300, Jarkko Sakkinen wrote:
> Would be probably easier to review also this way because the commit kind
> of rationalizes why things exist.
> 
> What do you think?

Sounds like a plan but you can do this for the next version - no need to
do it now. I'll try to review this way, per ioctl as I said in my mail
to Sean.

Thx.

-- 
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Jarkko Sakkinen]

On Thu, Jun 25, 2020 at 07:23:19PM +0200, Borislav Petkov wrote:
> On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> > Intel Software Guard eXtensions (SGX) is a set of CPU instructions that
> > can be used by applications to set aside private regions of code and
> > data. The code outside the SGX hosted software entity is disallowed to
> > access the memory inside the enclave enforced by the CPU. We call these
> > entities as enclaves.
> > 
> > This commit implements a driver that provides an ioctl API to construct
> > and run enclaves. Enclaves are constructed from pages residing in
> > reserved physical memory areas. The contents of these pages can only be
> > accessed when they are mapped as part of an enclave, by a hardware
> > thread running inside the enclave.
> > 
> > The starting state of an enclave consists of a fixed measured set of
> > pages that are copied to the EPC during the construction process by
> > using ENCLS leaf functions and Software Enclave Control Structure (SECS)
> > that defines the enclave properties.
> > 
> > Enclave are constructed by using ENCLS leaf functions ECREATE, EADD and
> > EINIT. ECREATE initializes SECS, EADD copies pages from system memory to
> > the EPC and EINIT check a given signed measurement and moves the enclave
> > into a state ready for execution.
> > 
> > An initialized enclave can only be accessed through special Thread Control
> > Structure (TCS) pages by using ENCLU (ring-3 only) leaf EENTER.  This leaf
> > function converts a thread into enclave mode and continues the execution in
> > the offset defined by the TCS provided to EENTER. An enclave is exited
> > through syscall, exception, interrupts or by explicitly calling another
> > ENCLU leaf EEXIT.
> > 
> > The permissions, which enclave page is added will set the limit for maximum
> > permissions that can be set for mmap() and mprotect(). This will
> > effectively allow to build different security schemes between producers and
> > consumers of enclaves. Later on we can increase granularity with LSM hooks
> > for page addition (i.e. for producers) and mapping of the enclave (i.e. for
> > consumers)
> > 
> > Cc: linux-security-mod...@vger.kernel.org
> > Acked-by: Jethro Beekman 
> > Tested-by: Jethro Beekman 
> > Tested-by: Haitao Huang 
> > Tested-by: Chunyang Hui 
> > Tested-by: Jordan Hand 
> > Tested-by: Nathaniel McCallum 
> > Tested-by: Seth Moore 
> > Co-developed-by: Sean Christopherson 
> > Signed-off-by: Sean Christopherson 
> > Co-developed-by: Suresh Siddha 
> > Signed-off-by: Suresh Siddha 
> > Signed-off-by: Jarkko Sakkinen 
> > ---
> >  .../userspace-api/ioctl/ioctl-number.rst  |   1 +
> >  arch/x86/include/uapi/asm/sgx.h   |  66 ++
> >  arch/x86/kernel/cpu/sgx/Makefile  |   3 +
> >  arch/x86/kernel/cpu/sgx/driver.c  | 194 +
> >  arch/x86/kernel/cpu/sgx/driver.h  |  30 +
> >  arch/x86/kernel/cpu/sgx/encl.c| 335 +
> >  arch/x86/kernel/cpu/sgx/encl.h|  87 +++
> >  arch/x86/kernel/cpu/sgx/ioctl.c   | 706 ++
> >  arch/x86/kernel/cpu/sgx/main.c|  11 +
> >  9 files changed, 1433 insertions(+)
> >  create mode 100644 arch/x86/include/uapi/asm/sgx.h
> >  create mode 100644 arch/x86/kernel/cpu/sgx/driver.c
> >  create mode 100644 arch/x86/kernel/cpu/sgx/driver.h
> >  create mode 100644 arch/x86/kernel/cpu/sgx/encl.c
> >  create mode 100644 arch/x86/kernel/cpu/sgx/encl.h
> >  create mode 100644 arch/x86/kernel/cpu/sgx/ioctl.c
> > 
> > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst 
> > b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > index 59472cd6a11d..35f713e3a267 100644
> > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > @@ -323,6 +323,7 @@ Code  Seq#Include File  
> >  Comments
> >   
> > 
> >  0xA3  90-9F  linux/dtlk.h
> >  0xA4  00-1F  uapi/linux/tee.h
> > Generic TEE subsystem
> > +0xA4  00-1F  uapi/asm/sgx.h  Intel 
> > SGX subsystem (a legit conflict as TEE and SGX do not co-exist)
> >  0xAA  00-3F  linux/uapi/linux/userfaultfd.h
> >  0xAB  00-1F  linux/nbd.h
> >  0xAC  00-1F  linux/raw.h
> > diff --git a/arch/x86/include/uapi/asm/sgx.h 
> > b/arch/x86/include/uapi/asm/sgx.h
> > new file mode 100644
> > index ..5edb08ab8fd0
> > --- /dev/null
> > +++ b/arch/x86/include/uapi/asm/sgx.h
> > @@ -0,0 +1,66 @@
> > +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH 
> > Linux-syscall-note */
> 
> Checkpatch complains here:
> 
> WARNING: 'SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH 
> Linux-syscall-note */' is not supported in LICENSES/...
> #114: FILE: arch/x86/include/uapi/asm/sgx.h:1:
> +/* 

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Borislav Petkov]

On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:

> Subject: Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver
 ^
 Add

> Intel Software Guard eXtensions (SGX) is a set of CPU instructions that
> can be used by applications to set aside private regions of code and
> data. The code outside the SGX hosted software entity is disallowed to
> access the memory inside the enclave enforced by the CPU. We call these
> entities as enclaves.

s/as //

> This commit implements a driver that provides an ioctl API to construct

s/This commit implements/Implement/

> and run enclaves. Enclaves are constructed from pages residing in
> reserved physical memory areas. The contents of these pages can only be
> accessed when they are mapped as part of an enclave, by a hardware
> thread running inside the enclave.
> 
> The starting state of an enclave consists of a fixed measured set of
> pages that are copied to the EPC during the construction process by
> using ENCLS leaf functions and Software Enclave Control Structure (SECS)
> that defines the enclave properties.
> 
> Enclave are constructed by using ENCLS leaf functions ECREATE, EADD and

Enclaves

> EINIT. ECREATE initializes SECS, EADD copies pages from system memory to
> the EPC and EINIT check a given signed measurement and moves the enclave

   checks

> into a state ready for execution.
> 
> An initialized enclave can only be accessed through special Thread Control
> Structure (TCS) pages by using ENCLU (ring-3 only) leaf EENTER.  This leaf
> function converts a thread into enclave mode and continues the execution in
> the offset defined by the TCS provided to EENTER. An enclave is exited
> through syscall, exception, interrupts or by explicitly calling another
> ENCLU leaf EEXIT.
> 
> The permissions, which enclave page is added will set the limit for maximum
> permissions that can be set for mmap() and mprotect().

I can't parse that sentence.

> This will
> effectively allow to build different security schemes between producers and
> consumers of enclaves. Later on we can increase granularity with LSM hooks
> for page addition (i.e. for producers) and mapping of the enclave (i.e. for
> consumers)

Other than that, nice explanation. I like that in a commit message.

Thx.

-- 
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Borislav Petkov]

On Thu, Jun 25, 2020 at 11:34:48AM -0700, Sean Christopherson wrote:
> Hmm, I think the most reasonable way to break up this beast would be to
> incrementally introduce functionality.  E.g. four or so patches, one for
> each ioctl() of ENCLAVE_CREATE, ENCLAVE_ADD_PAGES, ENCLAVE_INIT and
> ENCLAVE_SET_ATTRIBUTE, in that order.

Yeah, I guess I can try reviewing it this way too and address each ioctl
separately. You can try splitting later so that we don't waste time now.
It would be good to have it split eventually, though, so that it is more
palatable for other rewiewers too...

> Splitting up by file probably wouldn't work very well.  The split is
> pretty arbitrary, e.g. encl.[ch] isn't simply a pure representation of an
> enclave, there is a lot of the driver details/dependencies in there, i.e.
> the functionality between encl/ioctl/driver is all pretty intertwined.

... provided the functionality is not too intertwined to make a split
actually worse than a single big patch.

Thx.

-- 
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Sean Christopherson]

On Thu, Jun 25, 2020 at 07:23:19PM +0200, Borislav Petkov wrote:
> Also, you had all patches until now split nice and logically doing one
> thing only.
> 
> But this one is huge. Why?
> 
> Why can't you split out the facilities which the driver uses: encl.[ch]
> into a patch, then ioctl.c into a separate one and then the driver into
> a third one? Or do they all belong together inseparably?
> 
> I guess I'll find out eventually but it would've been nice if they were
> split out...

Hmm, I think the most reasonable way to break up this beast would be to
incrementally introduce functionality.  E.g. four or so patches, one for
each ioctl() of ENCLAVE_CREATE, ENCLAVE_ADD_PAGES, ENCLAVE_INIT and
ENCLAVE_SET_ATTRIBUTE, in that order.

Splitting up by file probably wouldn't work very well.  The split is
pretty arbitrary, e.g. encl.[ch] isn't simply a pure representation of an
enclave, there is a lot of the driver details/dependencies in there, i.e.
the functionality between encl/ioctl/driver is all pretty intertwined.

But I think serially introducing each ioctl() would be fairly clean, and
would help readers/reviewers better understand SGX as the patches would
naturally document the process of building an enclave, e.g. CREATE the
enclave, then ADD_PAGES, then INIT the enclave.  SET_ATTRIBUTE is a bit
of an outlier in that it would be chronologically out of order with
respect to building the enclave, but I think that's ok. 

Jarkko, thoughts?

Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Borislav Petkov]

On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> Intel Software Guard eXtensions (SGX) is a set of CPU instructions that
> can be used by applications to set aside private regions of code and
> data. The code outside the SGX hosted software entity is disallowed to
> access the memory inside the enclave enforced by the CPU. We call these
> entities as enclaves.
> 
> This commit implements a driver that provides an ioctl API to construct
> and run enclaves. Enclaves are constructed from pages residing in
> reserved physical memory areas. The contents of these pages can only be
> accessed when they are mapped as part of an enclave, by a hardware
> thread running inside the enclave.
> 
> The starting state of an enclave consists of a fixed measured set of
> pages that are copied to the EPC during the construction process by
> using ENCLS leaf functions and Software Enclave Control Structure (SECS)
> that defines the enclave properties.
> 
> Enclave are constructed by using ENCLS leaf functions ECREATE, EADD and
> EINIT. ECREATE initializes SECS, EADD copies pages from system memory to
> the EPC and EINIT check a given signed measurement and moves the enclave
> into a state ready for execution.
> 
> An initialized enclave can only be accessed through special Thread Control
> Structure (TCS) pages by using ENCLU (ring-3 only) leaf EENTER.  This leaf
> function converts a thread into enclave mode and continues the execution in
> the offset defined by the TCS provided to EENTER. An enclave is exited
> through syscall, exception, interrupts or by explicitly calling another
> ENCLU leaf EEXIT.
> 
> The permissions, which enclave page is added will set the limit for maximum
> permissions that can be set for mmap() and mprotect(). This will
> effectively allow to build different security schemes between producers and
> consumers of enclaves. Later on we can increase granularity with LSM hooks
> for page addition (i.e. for producers) and mapping of the enclave (i.e. for
> consumers)
> 
> Cc: linux-security-mod...@vger.kernel.org
> Acked-by: Jethro Beekman 
> Tested-by: Jethro Beekman 
> Tested-by: Haitao Huang 
> Tested-by: Chunyang Hui 
> Tested-by: Jordan Hand 
> Tested-by: Nathaniel McCallum 
> Tested-by: Seth Moore 
> Co-developed-by: Sean Christopherson 
> Signed-off-by: Sean Christopherson 
> Co-developed-by: Suresh Siddha 
> Signed-off-by: Suresh Siddha 
> Signed-off-by: Jarkko Sakkinen 
> ---
>  .../userspace-api/ioctl/ioctl-number.rst  |   1 +
>  arch/x86/include/uapi/asm/sgx.h   |  66 ++
>  arch/x86/kernel/cpu/sgx/Makefile  |   3 +
>  arch/x86/kernel/cpu/sgx/driver.c  | 194 +
>  arch/x86/kernel/cpu/sgx/driver.h  |  30 +
>  arch/x86/kernel/cpu/sgx/encl.c| 335 +
>  arch/x86/kernel/cpu/sgx/encl.h|  87 +++
>  arch/x86/kernel/cpu/sgx/ioctl.c   | 706 ++
>  arch/x86/kernel/cpu/sgx/main.c|  11 +
>  9 files changed, 1433 insertions(+)
>  create mode 100644 arch/x86/include/uapi/asm/sgx.h
>  create mode 100644 arch/x86/kernel/cpu/sgx/driver.c
>  create mode 100644 arch/x86/kernel/cpu/sgx/driver.h
>  create mode 100644 arch/x86/kernel/cpu/sgx/encl.c
>  create mode 100644 arch/x86/kernel/cpu/sgx/encl.h
>  create mode 100644 arch/x86/kernel/cpu/sgx/ioctl.c
> 
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst 
> b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 59472cd6a11d..35f713e3a267 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -323,6 +323,7 @@ Code  Seq#Include File
>Comments
>   
> 
>  0xA3  90-9F  linux/dtlk.h
>  0xA4  00-1F  uapi/linux/tee.hGeneric 
> TEE subsystem
> +0xA4  00-1F  uapi/asm/sgx.h  Intel 
> SGX subsystem (a legit conflict as TEE and SGX do not co-exist)
>  0xAA  00-3F  linux/uapi/linux/userfaultfd.h
>  0xAB  00-1F  linux/nbd.h
>  0xAC  00-1F  linux/raw.h
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> new file mode 100644
> index ..5edb08ab8fd0
> --- /dev/null
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH 
> Linux-syscall-note */

Checkpatch complains here:

WARNING: 'SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH 
Linux-syscall-note */' is not supported in LICENSES/...
#114: FILE: arch/x86/include/uapi/asm/sgx.h:1:
+/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note 
*/

Also, you had all patches until now split nice and logically doing one
thing only.

But this one is huge. Why?

Why can't you split out the facilities which the driver uses: encl.[ch]
into