[4/5] x86/sgx: Validate TCS permssions in sgx_validate_secinfo()
diff mbox series

Message ID 20190819152544.7296-5-jarkko.sakkinen@linux.intel.com
State New
Headers show
Series
  • x86/sgx: Improve permission handing
Related show

Commit Message

Jarkko Sakkinen Aug. 19, 2019, 3:25 p.m. UTC
The validation of TCS permissions was missing from
sgx_validate_secinfo(). This patch adds the validation.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Jarkko Sakkinen Aug. 21, 2019, 6:45 p.m. UTC | #1
On Mon, Aug 19, 2019 at 06:25:43PM +0300, Jarkko Sakkinen wrote:
> The validation of TCS permissions was missing from
> sgx_validate_secinfo(). This patch adds the validation.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index 99b1b9776c3a..2415dcb20a6e 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -423,6 +423,12 @@ static int sgx_validate_secinfo(struct sgx_secinfo *secinfo)
>  	if ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R))
>  		return -EINVAL;
>  
> +	/* CPU will silently overwrite the permissions as zero, which means
> +	 * that we need to validate it ourselves.
> +	 */
> +	if (page_type == SGX_SECINFO_TCS && perm)
> +		return -EINVAL;
> +
>  	if (secinfo->flags & SGX_SECINFO_RESERVED_MASK)
>  		return -EINVAL;
>  
> -- 
> 2.20.1
> 

OK, just found out that this patch did not end up to my test image and
causes a regression.

I think this should be fixed in sgx_encl_may_map() by having the
following special case for TCS (in addition to the change in this
patch of course):

1. Check if we encounter a TCS page.
2. If yes, we evaluate RW for the VM flags.

/Jarkko
Sean Christopherson Aug. 22, 2019, 3:55 a.m. UTC | #2
On Mon, Aug 19, 2019 at 06:25:43PM +0300, Jarkko Sakkinen wrote:
> The validation of TCS permissions was missing from
> sgx_validate_secinfo(). This patch adds the validation.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index 99b1b9776c3a..2415dcb20a6e 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -423,6 +423,12 @@ static int sgx_validate_secinfo(struct sgx_secinfo *secinfo)
>  	if ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R))
>  		return -EINVAL;
>  
> +	/* CPU will silently overwrite the permissions as zero, which means

Comment style again.  I looked it up just to double check :)


Documentation/process/coding-style.rst says:

When commenting the kernel API functions, please use the kernel-doc format.
See the files at :ref:`Documentation/doc-guide/ <doc_guide>` and
``scripts/kernel-doc`` for details.

The preferred style for long (multi-line) comments is:

.. code-block:: c

        /*
         * This is the preferred style for multi-line
         * comments in the Linux kernel source code.
         * Please use it consistently.
         *
         * Description:  A column of asterisks on the left side,
         * with beginning and ending almost-blank lines.
         */

For files in net/ and drivers/net/ the preferred style for long (multi-line)
comments is a little different.

.. code-block:: c

        /* The preferred comment style for files in net/ and drivers/net
         * looks like this.
         *
         * It is nearly the same as the generally preferred comment style,
         * but there is no initial almost-blank line.
         */


> +	 * that we need to validate it ourselves.
> +	 */
> +	if (page_type == SGX_SECINFO_TCS && perm)
> +		return -EINVAL;

Why are we validating the TCS protection bits?  Hardware ignores them, so
why do we care?  sgx_ioc_enclave_add_page() sets the internal protection
bits so there's no danger of putting the wrong thing in the page tables.

> +
>  	if (secinfo->flags & SGX_SECINFO_RESERVED_MASK)
>  		return -EINVAL;
>  
> -- 
> 2.20.1
>
Ayoun, Serge Aug. 22, 2019, 11:33 a.m. UTC | #3
> From: linux-sgx-owner@vger.kernel.org <linux-sgx-owner@vger.kernel.org>
> On Behalf Of Jarkko Sakkinen
> Sent: Wednesday, August 21, 2019 21:46
> To: linux-sgx@vger.kernel.org
> Cc: Christopherson, Sean J <sean.j.christopherson@intel.com>
> Subject: Re: [PATCH 4/5] x86/sgx: Validate TCS permssions in
> sgx_validate_secinfo()
> 
> On Mon, Aug 19, 2019 at 06:25:43PM +0300, Jarkko Sakkinen wrote:
> > The validation of TCS permissions was missing from
> > sgx_validate_secinfo(). This patch adds the validation.
> >
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > index 99b1b9776c3a..2415dcb20a6e 100644
> > --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > @@ -423,6 +423,12 @@ static int sgx_validate_secinfo(struct sgx_secinfo
> *secinfo)
> >  	if ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R))
> >  		return -EINVAL;
> >
> > +	/* CPU will silently overwrite the permissions as zero, which means
> > +	 * that we need to validate it ourselves.
> > +	 */
> > +	if (page_type == SGX_SECINFO_TCS && perm)
> > +		return -EINVAL;
> > +
> >  	if (secinfo->flags & SGX_SECINFO_RESERVED_MASK)
> >  		return -EINVAL;
> >
> > --
> > 2.20.1
> >
> 
> OK, just found out that this patch did not end up to my test image and
> causes a regression.
> 
> I think this should be fixed in sgx_encl_may_map() by having the
> following special case for TCS (in addition to the change in this
> patch of course):
> 
> 1. Check if we encounter a TCS page.
> 2. If yes, we evaluate RW for the VM flags.
> 

Also replying to Sean.
Sean is right that never mind the value in secsinfo->flags, HW will reset RWX
For TCS pages.
So basically you may not enforce and and could not check those but... The signature depends
On those flags, so if you put a non-zero flag value, eadd will pass but if you
compute the signature according to this non zero value then you will have
a delta between ur signature and HW's signature: einit will fail.
So this is tricky and more a usability issue.
I vote for checking the flag is zeroed.
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
Sean Christopherson Aug. 22, 2019, 2:27 p.m. UTC | #4
On Thu, Aug 22, 2019 at 04:33:35AM -0700, Ayoun, Serge wrote:
> Sean is right that never mind the value in secsinfo->flags, HW will reset RWX
> For TCS pages.  So basically you may not enforce and and could not check
> those but... The signature depends On those flags, so if you put a non-zero
> flag value, eadd will pass but if you compute the signature according to this
> non zero value then you will have a delta between ur signature and HW's
> signature: einit will fail.  So this is tricky and more a usability issue.  I
> vote for checking the flag is zeroed.

Ugh, didn't think about that behavior.  That's obnoxious.  Adding the
check makes sense in that case.
Jarkko Sakkinen Aug. 22, 2019, 4:31 p.m. UTC | #5
On Wed, 2019-08-21 at 20:55 -0700, Sean Christopherson wrote:
> Why are we validating the TCS protection bits?  Hardware ignores them, so
> why do we care?  sgx_ioc_enclave_add_page() sets the internal protection
> bits so there's no danger of putting the wrong thing in the page tables.

I think that in this commit I got it wrong but I think this is awkward:

	/*
	 * TCS pages must always RW set for CPU access while the SECINFO
	 * permissions are *always* zero - the CPU ignores the user provided
	 * values and silently overwrites with zero permissions.
	 */
	if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS)
		prot |= PROT_READ | PROT_WRITE;

In my opinion the right thing to do would be check that SECINFO has *at
minimum* RW and return -EINVAL if not.

I don't like the SGX silently adjusting permissions like this.

/Jarkko
Sean Christopherson Aug. 22, 2019, 4:34 p.m. UTC | #6
On Thu, Aug 22, 2019 at 07:31:39PM +0300, Jarkko Sakkinen wrote:
> On Wed, 2019-08-21 at 20:55 -0700, Sean Christopherson wrote:
> > Why are we validating the TCS protection bits?  Hardware ignores them, so
> > why do we care?  sgx_ioc_enclave_add_page() sets the internal protection
> > bits so there's no danger of putting the wrong thing in the page tables.
> 
> I think that in this commit I got it wrong but I think this is awkward:
> 
> 	/*
> 	 * TCS pages must always RW set for CPU access while the SECINFO
> 	 * permissions are *always* zero - the CPU ignores the user provided
> 	 * values and silently overwrites with zero permissions.
> 	 */
> 	if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS)
> 		prot |= PROT_READ | PROT_WRITE;
> 
> In my opinion the right thing to do would be check that SECINFO has *at
> minimum* RW and return -EINVAL if not.

Based on Serge's comment, hardware updates MRENCLAVE with SECINFO *after*
it overwrites the flags for TCS pages.  I.e. requiring RW for the TCS
would result in every enclave failing EINIT due to an invalid measurement.
It'd be fairly easy to verify this if we want to triple check that that is
indeed hardware behavior.
Jarkko Sakkinen Aug. 22, 2019, 4:38 p.m. UTC | #7
On Thu, 2019-08-22 at 19:31 +0300, Jarkko Sakkinen wrote:
> On Wed, 2019-08-21 at 20:55 -0700, Sean Christopherson wrote:
> > Why are we validating the TCS protection bits?  Hardware ignores them, so
> > why do we care?  sgx_ioc_enclave_add_page() sets the internal protection
> > bits so there's no danger of putting the wrong thing in the page tables.
> 
> I think that in this commit I got it wrong but I think this is awkward:
> 
> 	/*
> 	 * TCS pages must always RW set for CPU access while the SECINFO
> 	 * permissions are *always* zero - the CPU ignores the user provided
> 	 * values and silently overwrites with zero permissions.
> 	 */
> 	if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS)
> 		prot |= PROT_READ | PROT_WRITE;
> 
> In my opinion the right thing to do would be check that SECINFO has *at
> minimum* RW and return -EINVAL if not.
> 
> I don't like the SGX silently adjusting permissions like this.

For me any sane solution goes where we don't have that kind of tweaking
probably my "minimum RW" is more sane than my earlier proposal.

/Jarkko
Jarkko Sakkinen Aug. 22, 2019, 4:46 p.m. UTC | #8
On Thu, 2019-08-22 at 11:33 +0000, Ayoun, Serge wrote:
> Also replying to Sean.
> Sean is right that never mind the value in secsinfo->flags, HW will reset RWX
> For TCS pages.
> So basically you may not enforce and and could not check those but... The signature depends
> On those flags, so if you put a non-zero flag value, eadd will pass but if you
> compute the signature according to this non zero value then you will have
> a delta between ur signature and HW's signature: einit will fail.
> So this is tricky and more a usability issue.
> I vote for checking the flag is zeroed.

As I responded to Sean that as long as the ioctl does not adjust
prot bits I'm cool with any sane solution. What do you think of
requiring at minimum RW?

Doing that kind of adjusting is just doing fixup's for corrupted
data from the user space.

/Jarkko
Jarkko Sakkinen Aug. 22, 2019, 4:59 p.m. UTC | #9
On Thu, 2019-08-22 at 19:46 +0300, Jarkko Sakkinen wrote:
> On Thu, 2019-08-22 at 11:33 +0000, Ayoun, Serge wrote:
> > Also replying to Sean.
> > Sean is right that never mind the value in secsinfo->flags, HW will reset RWX
> > For TCS pages.
> > So basically you may not enforce and and could not check those but... The signature depends
> > On those flags, so if you put a non-zero flag value, eadd will pass but if you
> > compute the signature according to this non zero value then you will have
> > a delta between ur signature and HW's signature: einit will fail.
> > So this is tricky and more a usability issue.
> > I vote for checking the flag is zeroed.
> 
> As I responded to Sean that as long as the ioctl does not adjust
> prot bits I'm cool with any sane solution. What do you think of
> requiring at minimum RW?
> 
> Doing that kind of adjusting is just doing fixup's for corrupted
> data from the user space.

Kind of missed your comment about EINIT in rush! A valid point
and good catch.

I still think my 2nd proposal would be more appropriate than this
patch. Signatures will work and we don't need special cases anywhere.

/Jarkko
Jarkko Sakkinen Aug. 23, 2019, 12:39 a.m. UTC | #10
On Thu, 2019-08-22 at 09:34 -0700, Sean Christopherson wrote:
> On Thu, Aug 22, 2019 at 07:31:39PM +0300, Jarkko Sakkinen wrote:
> > On Wed, 2019-08-21 at 20:55 -0700, Sean Christopherson wrote:
> > > Why are we validating the TCS protection bits?  Hardware ignores them, so
> > > why do we care?  sgx_ioc_enclave_add_page() sets the internal protection
> > > bits so there's no danger of putting the wrong thing in the page tables.
> > 
> > I think that in this commit I got it wrong but I think this is awkward:
> > 
> > 	/*
> > 	 * TCS pages must always RW set for CPU access while the SECINFO
> > 	 * permissions are *always* zero - the CPU ignores the user provided
> > 	 * values and silently overwrites with zero permissions.
> > 	 */
> > 	if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS)
> > 		prot |= PROT_READ | PROT_WRITE;
> > 
> > In my opinion the right thing to do would be check that SECINFO has *at
> > minimum* RW and return -EINVAL if not.
> 
> Based on Serge's comment, hardware updates MRENCLAVE with SECINFO *after*
> it overwrites the flags for TCS pages.  I.e. requiring RW for the TCS
> would result in every enclave failing EINIT due to an invalid measurement.
> It'd be fairly easy to verify this if we want to triple check that that is
> indeed hardware behavior.

This is from the signing tool that I wrote back in 2016 used in the
selftest:

struct mreadd {
	uint64_t tag;
	uint64_t offset;
	uint64_t flags; /* SECINFO flags */
	uint8_t reserved[40];
} __attribute__((__packed__));

static bool mrenclave_eadd(EVP_MD_CTX *ctx, uint64_t offset, uint64_t flags)
{
	struct mreadd mreadd;

	memset(&mreadd, 0, sizeof(mreadd));
	mreadd.tag = MREADD;
	mreadd.offset = offset;
	mreadd.flags = flags;

	return mrenclave_update(ctx, &mreadd);
}

If MRENCLAVE was updated after the overwrite, this would not work.

The least confusing semantics would be to require RW, no more or less.

/Jarkko
Jarkko Sakkinen Aug. 23, 2019, 12:57 a.m. UTC | #11
On Fri, 2019-08-23 at 03:39 +0300, Jarkko Sakkinen wrote:
> On Thu, 2019-08-22 at 09:34 -0700, Sean Christopherson wrote:
> > On Thu, Aug 22, 2019 at 07:31:39PM +0300, Jarkko Sakkinen wrote:
> > > On Wed, 2019-08-21 at 20:55 -0700, Sean Christopherson wrote:
> > > > Why are we validating the TCS protection bits?  Hardware ignores them, so
> > > > why do we care?  sgx_ioc_enclave_add_page() sets the internal protection
> > > > bits so there's no danger of putting the wrong thing in the page tables.
> > > 
> > > I think that in this commit I got it wrong but I think this is awkward:
> > > 
> > > 	/*
> > > 	 * TCS pages must always RW set for CPU access while the SECINFO
> > > 	 * permissions are *always* zero - the CPU ignores the user provided
> > > 	 * values and silently overwrites with zero permissions.
> > > 	 */
> > > 	if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS)
> > > 		prot |= PROT_READ | PROT_WRITE;
> > > 
> > > In my opinion the right thing to do would be check that SECINFO has *at
> > > minimum* RW and return -EINVAL if not.
> > 
> > Based on Serge's comment, hardware updates MRENCLAVE with SECINFO *after*
> > it overwrites the flags for TCS pages.  I.e. requiring RW for the TCS
> > would result in every enclave failing EINIT due to an invalid measurement.
> > It'd be fairly easy to verify this if we want to triple check that that is
> > indeed hardware behavior.
> 
> This is from the signing tool that I wrote back in 2016 used in the
> selftest:
> 
> struct mreadd {
> 	uint64_t tag;
> 	uint64_t offset;
> 	uint64_t flags; /* SECINFO flags */
> 	uint8_t reserved[40];
> } __attribute__((__packed__));
> 
> static bool mrenclave_eadd(EVP_MD_CTX *ctx, uint64_t offset, uint64_t flags)
> {
> 	struct mreadd mreadd;
> 
> 	memset(&mreadd, 0, sizeof(mreadd));
> 	mreadd.tag = MREADD;
> 	mreadd.offset = offset;
> 	mreadd.flags = flags;
> 
> 	return mrenclave_update(ctx, &mreadd);
> }
> 
> If MRENCLAVE was updated after the overwrite, this would not work.
> 
> The least confusing semantics would be to require RW, no more or less.

OK, it is how Serge said.

This can we verified from the SDM easily (SCRATCH_SECINFO gets zeros
is extended after that).

And also from my signing tool :-)

for (offset = 0; offset < sb.st_size; offset += 0x1000) {
	if (!offset)
		flags = SGX_SECINFO_TCS;
	else
		flags = SGX_SECINFO_REG | SGX_SECINFO_R |
			SGX_SECINFO_W | SGX_SECINFO_X;

OK, so this looks like that my patch does exactly the right thing,
right?

/Jarkko
Sean Christopherson Aug. 23, 2019, 2:05 a.m. UTC | #12
On Fri, Aug 23, 2019 at 03:57:36AM +0300, Jarkko Sakkinen wrote:
> On Fri, 2019-08-23 at 03:39 +0300, Jarkko Sakkinen wrote:
> > On Thu, 2019-08-22 at 09:34 -0700, Sean Christopherson wrote:
> > > On Thu, Aug 22, 2019 at 07:31:39PM +0300, Jarkko Sakkinen wrote:
> > > > On Wed, 2019-08-21 at 20:55 -0700, Sean Christopherson wrote:
> > > > > Why are we validating the TCS protection bits?  Hardware ignores them, so
> > > > > why do we care?  sgx_ioc_enclave_add_page() sets the internal protection
> > > > > bits so there's no danger of putting the wrong thing in the page tables.
> > > > 
> > > > I think that in this commit I got it wrong but I think this is awkward:
> > > > 
> > > > 	/*
> > > > 	 * TCS pages must always RW set for CPU access while the SECINFO
> > > > 	 * permissions are *always* zero - the CPU ignores the user provided
> > > > 	 * values and silently overwrites with zero permissions.
> > > > 	 */
> > > > 	if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS)
> > > > 		prot |= PROT_READ | PROT_WRITE;
> > > > 
> > > > In my opinion the right thing to do would be check that SECINFO has *at
> > > > minimum* RW and return -EINVAL if not.
> > > 
> > > Based on Serge's comment, hardware updates MRENCLAVE with SECINFO *after*
> > > it overwrites the flags for TCS pages.  I.e. requiring RW for the TCS
> > > would result in every enclave failing EINIT due to an invalid measurement.
> > > It'd be fairly easy to verify this if we want to triple check that that is
> > > indeed hardware behavior.
> > 
> > This is from the signing tool that I wrote back in 2016 used in the
> > selftest:
> > 
> > struct mreadd {
> > 	uint64_t tag;
> > 	uint64_t offset;
> > 	uint64_t flags; /* SECINFO flags */
> > 	uint8_t reserved[40];
> > } __attribute__((__packed__));
> > 
> > static bool mrenclave_eadd(EVP_MD_CTX *ctx, uint64_t offset, uint64_t flags)
> > {
> > 	struct mreadd mreadd;
> > 
> > 	memset(&mreadd, 0, sizeof(mreadd));
> > 	mreadd.tag = MREADD;
> > 	mreadd.offset = offset;
> > 	mreadd.flags = flags;
> > 
> > 	return mrenclave_update(ctx, &mreadd);
> > }
> > 
> > If MRENCLAVE was updated after the overwrite, this would not work.
> > 
> > The least confusing semantics would be to require RW, no more or less.
> 
> OK, it is how Serge said.
> 
> This can we verified from the SDM easily (SCRATCH_SECINFO gets zeros
> is extended after that).
> 
> And also from my signing tool :-)
> 
> for (offset = 0; offset < sb.st_size; offset += 0x1000) {
> 	if (!offset)
> 		flags = SGX_SECINFO_TCS;
> 	else
> 		flags = SGX_SECINFO_REG | SGX_SECINFO_R |
> 			SGX_SECINFO_W | SGX_SECINFO_X;
> 
> OK, so this looks like that my patch does exactly the right thing,
> right?

That's my understanding as well.  Definitely worthy of a comment
explaining all of the above.
Jarkko Sakkinen Aug. 23, 2019, 1:41 p.m. UTC | #13
On Thu, 2019-08-22 at 19:05 -0700, Sean Christopherson wrote:
> > This can we verified from the SDM easily (SCRATCH_SECINFO gets zeros
> > is extended after that).
> > 
> > And also from my signing tool :-)
> > 
> > for (offset = 0; offset < sb.st_size; offset += 0x1000) {
> > 	if (!offset)
> > 		flags = SGX_SECINFO_TCS;
> > 	else
> > 		flags = SGX_SECINFO_REG | SGX_SECINFO_R |
> > 			SGX_SECINFO_W | SGX_SECINFO_X;
> > 
> > OK, so this looks like that my patch does exactly the right thing,
> > right?
> 
> That's my understanding as well.  Definitely worthy of a comment
> explaining all of the above.

Now that I looked at my own code I even remember going through this
same thought process three years ago when I wrote that :-) Oh well.

So should I apply my zero check patch?

/Jarkko

Patch
diff mbox series

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 99b1b9776c3a..2415dcb20a6e 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -423,6 +423,12 @@  static int sgx_validate_secinfo(struct sgx_secinfo *secinfo)
 	if ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R))
 		return -EINVAL;
 
+	/* CPU will silently overwrite the permissions as zero, which means
+	 * that we need to validate it ourselves.
+	 */
+	if (page_type == SGX_SECINFO_TCS && perm)
+		return -EINVAL;
+
 	if (secinfo->flags & SGX_SECINFO_RESERVED_MASK)
 		return -EINVAL;