| Message ID | 20201005020819.124724-1-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Remove checks for platform limits from sgx_validate_secs() | expand |
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 >
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
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
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...
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
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
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;
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(-)