* [PATCH] x86/sgx: Remove checks for platform limits from sgx_validate_secs() @ 2020-10-05 2:08 Jarkko Sakkinen 2020-10-05 2:29 ` Jarkko Sakkinen 2020-10-05 4:00 ` Haitao Huang 0 siblings, 2 replies; 7+ messages in thread From: Jarkko Sakkinen @ 2020-10-05 2:08 UTC (permalink / raw) To: linux-sgx Cc: Jarkko Sakkinen, Sean Christopherson, Jethro Beekman, Dave Hansen, Haitao Huang 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/sgx: Remove checks for platform limits from sgx_validate_secs() 2020-10-05 2:08 [PATCH] x86/sgx: Remove checks for platform limits from sgx_validate_secs() Jarkko Sakkinen @ 2020-10-05 2:29 ` Jarkko Sakkinen 2020-10-05 4:00 ` Haitao Huang 1 sibling, 0 replies; 7+ messages in thread From: Jarkko Sakkinen @ 2020-10-05 2:29 UTC (permalink / raw) To: linux-sgx; +Cc: Sean Christopherson, Jethro Beekman, Dave Hansen, Haitao Huang 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/sgx: Remove checks for platform limits from sgx_validate_secs() 2020-10-05 2:08 [PATCH] x86/sgx: Remove checks for platform limits from sgx_validate_secs() Jarkko Sakkinen 2020-10-05 2:29 ` Jarkko Sakkinen @ 2020-10-05 4:00 ` Haitao Huang 2020-10-05 11:38 ` Jarkko Sakkinen 1 sibling, 1 reply; 7+ messages in thread From: Haitao Huang @ 2020-10-05 4:00 UTC (permalink / raw) To: linux-sgx, Jarkko Sakkinen Cc: Sean Christopherson, Jethro Beekman, Dave Hansen 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/sgx: Remove checks for platform limits from sgx_validate_secs() 2020-10-05 4:00 ` Haitao Huang @ 2020-10-05 11:38 ` Jarkko Sakkinen 2020-10-05 22:46 ` Sean Christopherson 0 siblings, 1 reply; 7+ messages in thread From: Jarkko Sakkinen @ 2020-10-05 11:38 UTC (permalink / raw) To: Haitao Huang; +Cc: linux-sgx, Sean Christopherson, Jethro Beekman, Dave Hansen 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/sgx: Remove checks for platform limits from sgx_validate_secs() 2020-10-05 11:38 ` Jarkko Sakkinen @ 2020-10-05 22:46 ` Sean Christopherson 2020-10-06 15:16 ` Jarkko Sakkinen 0 siblings, 1 reply; 7+ messages in thread From: Sean Christopherson @ 2020-10-05 22:46 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: Haitao Huang, linux-sgx, Jethro Beekman, Dave Hansen 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... ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/sgx: Remove checks for platform limits from sgx_validate_secs() 2020-10-05 22:46 ` Sean Christopherson @ 2020-10-06 15:16 ` Jarkko Sakkinen 2020-10-06 15:32 ` Jarkko Sakkinen 0 siblings, 1 reply; 7+ messages in thread From: Jarkko Sakkinen @ 2020-10-06 15:16 UTC (permalink / raw) To: Sean Christopherson; +Cc: Haitao Huang, linux-sgx, Jethro Beekman, Dave Hansen 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/sgx: Remove checks for platform limits from sgx_validate_secs() 2020-10-06 15:16 ` Jarkko Sakkinen @ 2020-10-06 15:32 ` Jarkko Sakkinen 0 siblings, 0 replies; 7+ messages in thread From: Jarkko Sakkinen @ 2020-10-06 15:32 UTC (permalink / raw) To: Sean Christopherson; +Cc: Haitao Huang, linux-sgx, Jethro Beekman, Dave Hansen 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-10-06 15:37 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-05 2:08 [PATCH] x86/sgx: Remove checks for platform limits from sgx_validate_secs() Jarkko Sakkinen 2020-10-05 2:29 ` Jarkko Sakkinen 2020-10-05 4:00 ` Haitao Huang 2020-10-05 11:38 ` Jarkko Sakkinen 2020-10-05 22:46 ` Sean Christopherson 2020-10-06 15:16 ` Jarkko Sakkinen 2020-10-06 15:32 ` Jarkko Sakkinen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).