[PATCH] x86/sgx: Remove checks for platform limits from sgx_validate_secs()

[PATCH] x86/sgx: Remove checks for platform limits from sgx_validate_secs()

[Jarkko Sakkinen]

Remove from sgx_validate_secs():

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

SECS can surpass the platform limits because it's the SIGSTRUCT that
defines the limits that are used at run-time.

What SECS does is that it defines the overall limits that must apply for
any platform, i.e.  SECS limits and platform limits are orthogonal. They
are not dependent.

Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: Jethro Beekman <jethro@fortanix.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Suggested-by: Haitao Huang <haitao.huang@linux.intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/kernel/cpu/sgx/ioctl.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index 6b3cc8483008..008752cb54f0 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -86,11 +86,6 @@ static int sgx_validate_secs(const struct sgx_secs *secs)
 	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;
 
-- 
2.25.1

Re: [PATCH] x86/sgx: Remove checks for platform limits from sgx_validate_secs()

[Jarkko Sakkinen]

On Mon, Oct 05, 2020 at 05:08:19AM +0300, Jarkko Sakkinen wrote:
> Remove from sgx_validate_secs():
> 
> 	if (secs->miscselect & sgx_misc_reserved_mask ||
> 	    secs->attributes & sgx_attributes_reserved_mask ||
> 	    secs->xfrm & sgx_xfrm_reserved_mask)
> 		return -EINVAL;
> 
> SECS can surpass the platform limits because it's the SIGSTRUCT that
> defines the limits that are used at run-time.
> 
> What SECS does is that it defines the overall limits that must apply for
> any platform, i.e.  SECS limits and platform limits are orthogonal. They
> are not dependent.
> 
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Cc: Jethro Beekman <jethro@fortanix.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Suggested-by: Haitao Huang <haitao.huang@linux.intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

I think I got this now.

I think it would be less confusing if the variables were renamed as:

- sgx_cpu_misc
- sgx_cpu_attributes
- sgx_cpu_xfrm

These names nail their role better and do not imply any possible
dependency with SIGSTRUCT masks.

'cpu' is better than 'platform' because it is more exact.

Does anyone mind if I do these renames?

/Jarkko

> ---
>  arch/x86/kernel/cpu/sgx/ioctl.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> index 6b3cc8483008..008752cb54f0 100644
> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -86,11 +86,6 @@ static int sgx_validate_secs(const struct sgx_secs *secs)
>  	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;
>  
> -- 
> 2.25.1
> 

Re: [PATCH] x86/sgx: Remove checks for platform limits from sgx_validate_secs()

[Haitao Huang]

On Sun, 04 Oct 2020 21:08:19 -0500, Jarkko Sakkinen  
<jarkko.sakkinen@linux.intel.com> wrote:

> Remove from sgx_validate_secs():
>
> 	if (secs->miscselect & sgx_misc_reserved_mask ||
> 	    secs->attributes & sgx_attributes_reserved_mask ||
> 	    secs->xfrm & sgx_xfrm_reserved_mask)
> 		return -EINVAL;
>
> SECS can surpass the platform limits because it's the SIGSTRUCT that
> defines the limits that are used at run-time.
>
> What SECS does is that it defines the overall limits that must apply for
> any platform, i.e.  SECS limits and platform limits are orthogonal. They
> are not dependent.
>
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Cc: Jethro Beekman <jethro@fortanix.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Suggested-by: Haitao Huang <haitao.huang@linux.intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/ioctl.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c  
> b/arch/x86/kernel/cpu/sgx/ioctl.c
> index 6b3cc8483008..008752cb54f0 100644
> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -86,11 +86,6 @@ static int sgx_validate_secs(const struct sgx_secs  
> *secs)
>  	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;
>

Looks good to me.
Thanks
Haitao

Re: [PATCH] x86/sgx: Remove checks for platform limits from sgx_validate_secs()

[Jarkko Sakkinen]

On Sun, Oct 04, 2020 at 11:00:28PM -0500, Haitao Huang wrote:
> On Sun, 04 Oct 2020 21:08:19 -0500, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> 
> > Remove from sgx_validate_secs():
> > 
> > 	if (secs->miscselect & sgx_misc_reserved_mask ||
> > 	    secs->attributes & sgx_attributes_reserved_mask ||
> > 	    secs->xfrm & sgx_xfrm_reserved_mask)
> > 		return -EINVAL;
> > 
> > SECS can surpass the platform limits because it's the SIGSTRUCT that
> > defines the limits that are used at run-time.
> > 
> > What SECS does is that it defines the overall limits that must apply for
> > any platform, i.e.  SECS limits and platform limits are orthogonal. They
> > are not dependent.
> > 
> > Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> > Cc: Jethro Beekman <jethro@fortanix.com>
> > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > Suggested-by: Haitao Huang <haitao.huang@linux.intel.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  arch/x86/kernel/cpu/sgx/ioctl.c | 5 -----
> >  1 file changed, 5 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c
> > b/arch/x86/kernel/cpu/sgx/ioctl.c
> > index 6b3cc8483008..008752cb54f0 100644
> > --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> > @@ -86,11 +86,6 @@ static int sgx_validate_secs(const struct sgx_secs
> > *secs)
> >  	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;
> > 
> 
> Looks good to me.
> Thanks
> Haitao

Thanks, I'll merge it then.

/JArkko

Re: [PATCH] x86/sgx: Remove checks for platform limits from sgx_validate_secs()

[Sean Christopherson]

On Mon, Oct 05, 2020 at 02:38:40PM +0300, Jarkko Sakkinen wrote:
> On Sun, Oct 04, 2020 at 11:00:28PM -0500, Haitao Huang wrote:
> > On Sun, 04 Oct 2020 21:08:19 -0500, Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > 
> > > Remove from sgx_validate_secs():
> > > 
> > > 	if (secs->miscselect & sgx_misc_reserved_mask ||
> > > 	    secs->attributes & sgx_attributes_reserved_mask ||
> > > 	    secs->xfrm & sgx_xfrm_reserved_mask)
> > > 		return -EINVAL;
> > > 
> > > SECS can surpass the platform limits because it's the SIGSTRUCT that
> > > defines the limits that are used at run-time.
> > > 
> > > What SECS does is that it defines the overall limits that must apply for
> > > any platform, i.e.  SECS limits and platform limits are orthogonal. They
> > > are not dependent.
> > > 
> > > Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> > > Cc: Jethro Beekman <jethro@fortanix.com>
> > > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > > Suggested-by: Haitao Huang <haitao.huang@linux.intel.com>
> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > ---
> > >  arch/x86/kernel/cpu/sgx/ioctl.c | 5 -----
> > >  1 file changed, 5 deletions(-)
> > > 
> > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c
> > > b/arch/x86/kernel/cpu/sgx/ioctl.c
> > > index 6b3cc8483008..008752cb54f0 100644
> > > --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> > > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> > > @@ -86,11 +86,6 @@ static int sgx_validate_secs(const struct sgx_secs
> > > *secs)
> > >  	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;
> > > 
> > 
> > Looks good to me.
> > Thanks
> > Haitao
> 
> Thanks, I'll merge it then.

You can't remove this wholesale, the kernel should still disallow access to
features that are unknown to the kernel and/or are explicitly disallowed by
the kernel.  E.g. see SGX_ATTR_RESERVED_MASK and SGX_MISC_RESERVED_MASK.

Dropping sgx_xfrm_reserved_mask is ok because the CPU explicitly checks that
XFRM is a strict subset of the current XCR0, though that makes me wonder what
it does with XSS...

Re: [PATCH] x86/sgx: Remove checks for platform limits from sgx_validate_secs()

[Jarkko Sakkinen]

On Mon, Oct 05, 2020 at 03:46:26PM -0700, Sean Christopherson wrote:
> On Mon, Oct 05, 2020 at 02:38:40PM +0300, Jarkko Sakkinen wrote:
> > On Sun, Oct 04, 2020 at 11:00:28PM -0500, Haitao Huang wrote:
> > > On Sun, 04 Oct 2020 21:08:19 -0500, Jarkko Sakkinen
> > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > 
> > > > Remove from sgx_validate_secs():
> > > > 
> > > > 	if (secs->miscselect & sgx_misc_reserved_mask ||
> > > > 	    secs->attributes & sgx_attributes_reserved_mask ||
> > > > 	    secs->xfrm & sgx_xfrm_reserved_mask)
> > > > 		return -EINVAL;
> > > > 
> > > > SECS can surpass the platform limits because it's the SIGSTRUCT that
> > > > defines the limits that are used at run-time.
> > > > 
> > > > What SECS does is that it defines the overall limits that must apply for
> > > > any platform, i.e.  SECS limits and platform limits are orthogonal. They
> > > > are not dependent.
> > > > 
> > > > Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> > > > Cc: Jethro Beekman <jethro@fortanix.com>
> > > > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > > > Suggested-by: Haitao Huang <haitao.huang@linux.intel.com>
> > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > ---
> > > >  arch/x86/kernel/cpu/sgx/ioctl.c | 5 -----
> > > >  1 file changed, 5 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c
> > > > b/arch/x86/kernel/cpu/sgx/ioctl.c
> > > > index 6b3cc8483008..008752cb54f0 100644
> > > > --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> > > > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> > > > @@ -86,11 +86,6 @@ static int sgx_validate_secs(const struct sgx_secs
> > > > *secs)
> > > >  	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;
> > > > 
> > > 
> > > Looks good to me.
> > > Thanks
> > > Haitao
> > 
> > Thanks, I'll merge it then.
> 
> You can't remove this wholesale, the kernel should still disallow access to
> features that are unknown to the kernel and/or are explicitly disallowed by
> the kernel.  E.g. see SGX_ATTR_RESERVED_MASK and SGX_MISC_RESERVED_MASK.
> 
> Dropping sgx_xfrm_reserved_mask is ok because the CPU explicitly checks that
> XFRM is a strict subset of the current XCR0, though that makes me wonder what
> it does with XSS...

If possible send a patch so I can get a changelog reference.

Only thing you need to put as long description is permlink to my
response in this thread.

Thank you.

/Jarkko

Re: [PATCH] x86/sgx: Remove checks for platform limits from sgx_validate_secs()

[Jarkko Sakkinen]

On Tue, Oct 06, 2020 at 06:16:17PM +0300, Jarkko Sakkinen wrote:
> On Mon, Oct 05, 2020 at 03:46:26PM -0700, Sean Christopherson wrote:
> > On Mon, Oct 05, 2020 at 02:38:40PM +0300, Jarkko Sakkinen wrote:
> > > On Sun, Oct 04, 2020 at 11:00:28PM -0500, Haitao Huang wrote:
> > > > On Sun, 04 Oct 2020 21:08:19 -0500, Jarkko Sakkinen
> > > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > > 
> > > > > Remove from sgx_validate_secs():
> > > > > 
> > > > > 	if (secs->miscselect & sgx_misc_reserved_mask ||
> > > > > 	    secs->attributes & sgx_attributes_reserved_mask ||
> > > > > 	    secs->xfrm & sgx_xfrm_reserved_mask)
> > > > > 		return -EINVAL;
> > > > > 
> > > > > SECS can surpass the platform limits because it's the SIGSTRUCT that
> > > > > defines the limits that are used at run-time.
> > > > > 
> > > > > What SECS does is that it defines the overall limits that must apply for
> > > > > any platform, i.e.  SECS limits and platform limits are orthogonal. They
> > > > > are not dependent.
> > > > > 
> > > > > Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> > > > > Cc: Jethro Beekman <jethro@fortanix.com>
> > > > > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > > > > Suggested-by: Haitao Huang <haitao.huang@linux.intel.com>
> > > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > ---
> > > > >  arch/x86/kernel/cpu/sgx/ioctl.c | 5 -----
> > > > >  1 file changed, 5 deletions(-)
> > > > > 
> > > > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c
> > > > > b/arch/x86/kernel/cpu/sgx/ioctl.c
> > > > > index 6b3cc8483008..008752cb54f0 100644
> > > > > --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> > > > > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> > > > > @@ -86,11 +86,6 @@ static int sgx_validate_secs(const struct sgx_secs
> > > > > *secs)
> > > > >  	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;
> > > > > 
> > > > 
> > > > Looks good to me.
> > > > Thanks
> > > > Haitao
> > > 
> > > Thanks, I'll merge it then.
> > 
> > You can't remove this wholesale, the kernel should still disallow access to
> > features that are unknown to the kernel and/or are explicitly disallowed by
> > the kernel.  E.g. see SGX_ATTR_RESERVED_MASK and SGX_MISC_RESERVED_MASK.
> > 
> > Dropping sgx_xfrm_reserved_mask is ok because the CPU explicitly checks that
> > XFRM is a strict subset of the current XCR0, though that makes me wonder what
> > it does with XSS...
> 
> If possible send a patch so I can get a changelog reference.
> 
> Only thing you need to put as long description is permlink to my
> response in this thread.

Sean, I can do the fix, don't worry about it but before I make a backlog
entry I need to challenge this just a bit.

SIGSTRUCT will tell what features we want to run the enclave with, not
SECS, so why you see it a problem that the features are not validated
in SGX_IOC_ENCLAVE_CREATE? I.e. the enclave binary could potentially be
used in different platforms with different kernel versions.

/Jarkko