[PATCH 0/5] x86/sgx: Improve permission handing

[PATCH 0/5] x86/sgx: Improve permission handing

[Jarkko Sakkinen]

Refine the naming, documentation and validation of the page permissions.
silent behaviour to occur.

Cc: Sean Christopherson <sean.j.christpherson@intel.com>
Cc: Shay Katz-zamir <shay.katz-zamir@intel.com>
Cc: Serge Ayoun <serge.ayoun@intel.com>

Jarkko Sakkinen (5):
  x86/sgx: Document permission handling better
  x86/sgx: Use memchr_inv() in sgx_validate_secinfo()
  x86/sgx: Make sgx_validate_secinfo() more readable
  x86/sgx: Validate TCS permssions in sgx_validate_secinfo()
  x86/sgx: Rename vm_prot_bits as max_vm_flags

 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 36 +++++++++++++++++++-------
 arch/x86/kernel/cpu/sgx/encl.c         | 14 +++++-----
 arch/x86/kernel/cpu/sgx/encl.h         |  4 +--
 3 files changed, 35 insertions(+), 19 deletions(-)

-- 
2.20.1

[PATCH 1/5] x86/sgx: Document permission handling better

[Jarkko Sakkinen]

The way permissions are managed inside the driver is not trivial and
intuitive. The non-obvious parts were not properly remarked in the
source code. This patch refines them a bit.

Cc: Sean Christopherson <sean.j.christpherson@intel.com>
Cc: Shay Katz-zamir <shay.katz-zamir@intel.com>
Cc: Serge Ayoun <serge.ayoun@intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 9b784a061a47..64d3286f3324 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -259,7 +259,10 @@ static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
 		return ERR_PTR(-ENOMEM);
 	encl_page->desc = addr;
 	encl_page->encl = encl;
+
+	/* Calculate maximum of the VM flags for the page. */
 	encl_page->vm_prot_bits = calc_vm_prot_bits(prot, 0);
+
 	ret = radix_tree_insert(&encl->page_tree, PFN_DOWN(encl_page->desc),
 				encl_page);
 	if (ret) {
@@ -640,11 +643,15 @@ static long sgx_ioc_enclave_add_page(struct file *filep, void __user *arg)
 
 	data = kmap(data_page);
 
+	/* Set prot bits matching to the SECINFO permissions. */
 	prot = _calc_vm_trans(secinfo.flags, SGX_SECINFO_R, PROT_READ)  |
 	       _calc_vm_trans(secinfo.flags, SGX_SECINFO_W, PROT_WRITE) |
 	       _calc_vm_trans(secinfo.flags, SGX_SECINFO_X, PROT_EXEC);
 
-	/* TCS pages need to be RW in the PTEs, but can be 0 in the EPCM. */
+	/* 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 zero permissions.
+	 */
 	if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS)
 		prot |= PROT_READ | PROT_WRITE;
 
-- 
2.20.1

Re: [PATCH 1/5] x86/sgx: Document permission handling better

[Sean Christopherson]

On Mon, Aug 19, 2019 at 06:25:40PM +0300, Jarkko Sakkinen wrote:
> The way permissions are managed inside the driver is not trivial and
> intuitive. The non-obvious parts were not properly remarked in the
> source code. This patch refines them a bit.
> 
> Cc: Sean Christopherson <sean.j.christpherson@intel.com>
> Cc: Shay Katz-zamir <shay.katz-zamir@intel.com>
> Cc: Serge Ayoun <serge.ayoun@intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index 9b784a061a47..64d3286f3324 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -259,7 +259,10 @@ static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
>  		return ERR_PTR(-ENOMEM);
>  	encl_page->desc = addr;
>  	encl_page->encl = encl;
> +
> +	/* Calculate maximum of the VM flags for the page. */
>  	encl_page->vm_prot_bits = calc_vm_prot_bits(prot, 0);
> +
>  	ret = radix_tree_insert(&encl->page_tree, PFN_DOWN(encl_page->desc),
>  				encl_page);
>  	if (ret) {
> @@ -640,11 +643,15 @@ static long sgx_ioc_enclave_add_page(struct file *filep, void __user *arg)
>  
>  	data = kmap(data_page);
>  
> +	/* Set prot bits matching to the SECINFO permissions. */
>  	prot = _calc_vm_trans(secinfo.flags, SGX_SECINFO_R, PROT_READ)  |
>  	       _calc_vm_trans(secinfo.flags, SGX_SECINFO_W, PROT_WRITE) |
>  	       _calc_vm_trans(secinfo.flags, SGX_SECINFO_X, PROT_EXEC);
>  
> -	/* TCS pages need to be RW in the PTEs, but can be 0 in the EPCM. */
> +	/* TCS pages must always RW set for CPU access while the SECINFO

Should be

	/*
	 * TCS pages ...

> +	 * permissions are *always* zero - the CPU ignores the user provided
> +	 * values and silently overwrites zero permissions.

Maybe 'overwrites with zero permissions'?

> +	 */
>  	if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS)
>  		prot |= PROT_READ | PROT_WRITE;
>  
> -- 
> 2.20.1
> 

Re: [PATCH 1/5] x86/sgx: Document permission handling better

[Jarkko Sakkinen]

On Wed, 2019-08-21 at 20:43 -0700, Sean Christopherson wrote:
> > -	/* TCS pages need to be RW in the PTEs, but can be 0 in the EPCM. */
> > +	/* TCS pages must always RW set for CPU access while the SECINFO
> 
> Should be
> 
> 	/*
> 	 * TCS pages ...
> 
> > +	 * permissions are *always* zero - the CPU ignores the user provided
> > +	 * values and silently overwrites zero permissions.
> 
> Maybe 'overwrites with zero permissions'?

I squashed this with the proposed changes made. Thank you.

/Jarkko

[PATCH 2/5] x86/sgx: Use memchr_inv() in sgx_validate_secinfo()

[Jarkko Sakkinen]

Instead of looping through the array of reserved bytes use memchr_inv()
to check the bytes.

Cc: Sean Christopherson <sean.j.christpherson@intel.com>
Cc: Shay Katz-zamir <shay.katz-zamir@intel.com>
Cc: Serge Ayoun <serge.ayoun@intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 64d3286f3324..d5f326411df0 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -414,7 +414,6 @@ static int sgx_validate_secinfo(struct sgx_secinfo *secinfo)
 {
 	u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK;
 	u64 perm = secinfo->flags & SGX_SECINFO_PERMISSION_MASK;
-	int i;
 
 	if ((secinfo->flags & SGX_SECINFO_RESERVED_MASK) ||
 	    ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R)) ||
@@ -422,9 +421,8 @@ static int sgx_validate_secinfo(struct sgx_secinfo *secinfo)
 	     page_type != SGX_SECINFO_REG))
 		return -EINVAL;
 
-	for (i = 0; i < SGX_SECINFO_RESERVED_SIZE; i++)
-		if (secinfo->reserved[i])
-			return -EINVAL;
+	if (memchr_inv(secinfo->reserved, 0, SGX_SECINFO_RESERVED_SIZE))
+		return -EINVAL;
 
 	return 0;
 }
-- 
2.20.1

Re: [PATCH 2/5] x86/sgx: Use memchr_inv() in sgx_validate_secinfo()

[Sean Christopherson]

On Mon, Aug 19, 2019 at 06:25:41PM +0300, Jarkko Sakkinen wrote:
> Instead of looping through the array of reserved bytes use memchr_inv()
> to check the bytes.
> 
> Cc: Sean Christopherson <sean.j.christpherson@intel.com>
> Cc: Shay Katz-zamir <shay.katz-zamir@intel.com>
> Cc: Serge Ayoun <serge.ayoun@intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index 64d3286f3324..d5f326411df0 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -414,7 +414,6 @@ static int sgx_validate_secinfo(struct sgx_secinfo *secinfo)
>  {
>  	u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK;
>  	u64 perm = secinfo->flags & SGX_SECINFO_PERMISSION_MASK;
> -	int i;
>  
>  	if ((secinfo->flags & SGX_SECINFO_RESERVED_MASK) ||
>  	    ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R)) ||
> @@ -422,9 +421,8 @@ static int sgx_validate_secinfo(struct sgx_secinfo *secinfo)
>  	     page_type != SGX_SECINFO_REG))
>  		return -EINVAL;
>  
> -	for (i = 0; i < SGX_SECINFO_RESERVED_SIZE; i++)
> -		if (secinfo->reserved[i])
> -			return -EINVAL;
> +	if (memchr_inv(secinfo->reserved, 0, SGX_SECINFO_RESERVED_SIZE))

Doing 'sizeof(secinfo->reserved)' would be preferable, that way we're not
dependent on SGX_SECINFO_RESERVED_SIZE being in bytes (I had to check).

Obviously not in this patch, but the same cleanup can be applied to
sgx_validate_secs().

> +		return -EINVAL;
>  
>  	return 0;
>  }
> -- 
> 2.20.1
> 

Re: [PATCH 2/5] x86/sgx: Use memchr_inv() in sgx_validate_secinfo()

[Jarkko Sakkinen]

On Wed, 2019-08-21 at 20:47 -0700, Sean Christopherson wrote:
> > +	if (memchr_inv(secinfo->reserved, 0, SGX_SECINFO_RESERVED_SIZE))
> 
> Doing 'sizeof(secinfo->reserved)' would be preferable, that way we're not
> dependent on SGX_SECINFO_RESERVED_SIZE being in bytes (I had to check).
> 
> Obviously not in this patch, but the same cleanup can be applied to
> sgx_validate_secs().

Thanks for the valid remark, I squashed with that change. I also
edited prepending commit and removed the constant altogether.

/Jarkko

[PATCH 3/5] x86/sgx: Make sgx_validate_secinfo() more readable

[Jarkko Sakkinen]

Split the huge conditional statement to three separate ones in
order to make it easier to understand what is going on in the
validation code.

Cc: Sean Christopherson <sean.j.christpherson@intel.com>
Cc: Shay Katz-zamir <shay.katz-zamir@intel.com>
Cc: Serge Ayoun <serge.ayoun@intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index d5f326411df0..99b1b9776c3a 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -415,10 +415,15 @@ static int sgx_validate_secinfo(struct sgx_secinfo *secinfo)
 	u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK;
 	u64 perm = secinfo->flags & SGX_SECINFO_PERMISSION_MASK;
 
-	if ((secinfo->flags & SGX_SECINFO_RESERVED_MASK) ||
-	    ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R)) ||
-	    (page_type != SGX_SECINFO_TCS && page_type != SGX_SECINFO_TRIM &&
-	     page_type != SGX_SECINFO_REG))
+	if ((page_type != SGX_SECINFO_REG &&
+	     page_type != SGX_SECINFO_TCS &&
+	     page_type != SGX_SECINFO_TRIM))
+		return -EINVAL;
+
+	if ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R))
+		return -EINVAL;
+
+	if (secinfo->flags & SGX_SECINFO_RESERVED_MASK)
 		return -EINVAL;
 
 	if (memchr_inv(secinfo->reserved, 0, SGX_SECINFO_RESERVED_SIZE))
-- 
2.20.1

Re: [PATCH 3/5] x86/sgx: Make sgx_validate_secinfo() more readable

[Sean Christopherson]

On Mon, Aug 19, 2019 at 06:25:42PM +0300, Jarkko Sakkinen wrote:
> Split the huge conditional statement to three separate ones in
> order to make it easier to understand what is going on in the
> validation code.
> 
> Cc: Sean Christopherson <sean.j.christpherson@intel.com>
> Cc: Shay Katz-zamir <shay.katz-zamir@intel.com>
> Cc: Serge Ayoun <serge.ayoun@intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index d5f326411df0..99b1b9776c3a 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -415,10 +415,15 @@ static int sgx_validate_secinfo(struct sgx_secinfo *secinfo)
>  	u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK;
>  	u64 perm = secinfo->flags & SGX_SECINFO_PERMISSION_MASK;
>  
> -	if ((secinfo->flags & SGX_SECINFO_RESERVED_MASK) ||
> -	    ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R)) ||
> -	    (page_type != SGX_SECINFO_TCS && page_type != SGX_SECINFO_TRIM &&
> -	     page_type != SGX_SECINFO_REG))
> +	if ((page_type != SGX_SECINFO_REG &&
> +	     page_type != SGX_SECINFO_TCS &&
> +	     page_type != SGX_SECINFO_TRIM))

Shouldn't we disallow TRIM until SGX2 is supported?

> +		return -EINVAL;
> +
> +	if ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R))
> +		return -EINVAL;
> +
> +	if (secinfo->flags & SGX_SECINFO_RESERVED_MASK)
>  		return -EINVAL;
>  
>  	if (memchr_inv(secinfo->reserved, 0, SGX_SECINFO_RESERVED_SIZE))
> -- 
> 2.20.1
> 

Re: [PATCH 3/5] x86/sgx: Make sgx_validate_secinfo() more readable

[Jarkko Sakkinen]

On Wed, 2019-08-21 at 20:48 -0700, Sean Christopherson wrote:
> > +	if ((page_type != SGX_SECINFO_REG &&
> > +	     page_type != SGX_SECINFO_TCS &&
> > +	     page_type != SGX_SECINFO_TRIM))
> 
> Shouldn't we disallow TRIM until SGX2 is supported?

Yes.

I squashed the change with TRIM removed.

I also renamed the local variable page_type as pt. I think that is
a good abbrevation for page type and saves some screen space.

/Jarkko

RE: [PATCH 3/5] x86/sgx: Make sgx_validate_secinfo() more readable

[Ayoun, Serge]

> From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Sent: Monday, August 19, 2019 18:26
> To: linux-sgx@vger.kernel.org
> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; Sean Christopherson
> <sean.j.christpherson@intel.com>; Katz-zamir, Shay <shay.katz-
> zamir@intel.com>; Ayoun, Serge <serge.ayoun@intel.com>
> Subject: [PATCH 3/5] x86/sgx: Make sgx_validate_secinfo() more readable
> 
> Split the huge conditional statement to three separate ones in order to make
> it easier to understand what is going on in the validation code.
> 
> Cc: Sean Christopherson <sean.j.christpherson@intel.com>
> Cc: Shay Katz-zamir <shay.katz-zamir@intel.com>
> Cc: Serge Ayoun <serge.ayoun@intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index d5f326411df0..99b1b9776c3a 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -415,10 +415,15 @@ static int sgx_validate_secinfo(struct sgx_secinfo
> *secinfo)
>  	u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK;
>  	u64 perm = secinfo->flags & SGX_SECINFO_PERMISSION_MASK;
> 
> -	if ((secinfo->flags & SGX_SECINFO_RESERVED_MASK) ||
> -	    ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R)) ||
> -	    (page_type != SGX_SECINFO_TCS && page_type !=
> SGX_SECINFO_TRIM &&
> -	     page_type != SGX_SECINFO_REG))
> +	if ((page_type != SGX_SECINFO_REG &&
> +	     page_type != SGX_SECINFO_TCS &&
> +	     page_type != SGX_SECINFO_TRIM))
> +		return -EINVAL;

sgx_validate_secinfo() is called via eadd ioctl. Eadd will fail with
TRIM page type, so you probably need to remove it from the if
sgx2.0 does not change this behavior

> +
> +	if ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R))
> +		return -EINVAL;
> +
> +	if (secinfo->flags & SGX_SECINFO_RESERVED_MASK)
>  		return -EINVAL;
> 
>  	if (memchr_inv(secinfo->reserved, 0,
> SGX_SECINFO_RESERVED_SIZE))
> --
> 2.20.1

---------------------------------------------------------------------
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.

Re: [PATCH 3/5] x86/sgx: Make sgx_validate_secinfo() more readable

[Jarkko Sakkinen]

On Thu, 2019-08-22 at 10:39 +0000, Ayoun, Serge wrote:
> > +	if ((page_type != SGX_SECINFO_REG &&
> > +	     page_type != SGX_SECINFO_TCS &&
> > +	     page_type != SGX_SECINFO_TRIM))
> > +		return -EINVAL;
> 
> sgx_validate_secinfo() is called via eadd ioctl. Eadd will fail with
> TRIM page type, so you probably need to remove it from the if
> sgx2.0 does not change this behavior

Thanks! Removed.

/Jarkko

[PATCH 4/5] x86/sgx: Validate TCS permssions in sgx_validate_secinfo()

[Jarkko Sakkinen]

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

Re: [PATCH 4/5] x86/sgx: Validate TCS permssions in sgx_validate_secinfo()

[Jarkko Sakkinen]

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

RE: [PATCH 4/5] x86/sgx: Validate TCS permssions in sgx_validate_secinfo()

[Ayoun, Serge]

> 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.

Re: [PATCH 4/5] x86/sgx: Validate TCS permssions in sgx_validate_secinfo()

[Sean Christopherson]

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.

Re: [PATCH 4/5] x86/sgx: Validate TCS permssions in sgx_validate_secinfo()

[Jarkko Sakkinen]

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

Re: [PATCH 4/5] x86/sgx: Validate TCS permssions in sgx_validate_secinfo()

[Jarkko Sakkinen]

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

Re: [PATCH 4/5] x86/sgx: Validate TCS permssions in sgx_validate_secinfo()

[Sean Christopherson]

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
> 

Re: [PATCH 4/5] x86/sgx: Validate TCS permssions in sgx_validate_secinfo()

[Jarkko Sakkinen]

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

Re: [PATCH 4/5] x86/sgx: Validate TCS permssions in sgx_validate_secinfo()

[Sean Christopherson]

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.

Re: [PATCH 4/5] x86/sgx: Validate TCS permssions in sgx_validate_secinfo()

[Jarkko Sakkinen]

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

Re: [PATCH 4/5] x86/sgx: Validate TCS permssions in sgx_validate_secinfo()

[Jarkko Sakkinen]

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

Re: [PATCH 4/5] x86/sgx: Validate TCS permssions in sgx_validate_secinfo()

[Sean Christopherson]

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.

Re: [PATCH 4/5] x86/sgx: Validate TCS permssions in sgx_validate_secinfo()

[Jarkko Sakkinen]

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

Re: [PATCH 4/5] x86/sgx: Validate TCS permssions in sgx_validate_secinfo()

[Jarkko Sakkinen]

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

[PATCH 5/5] x86/sgx: Rename vm_prot_bits as max_vm_flags

[Jarkko Sakkinen]

vm_prot_bits is very bad and misleading name for the field in struct
sgx_encl_page. What the field contains exactly is not @prot of
mprotect() but the *maximum* VM flags for the VMA that contains the
given enclave page.

Thus, the only viable name for the field is max_vm_flags. In functions
that also pass VM flags the parameter name is renamed from vm_prot_bits
to vm_flags.

To summarize, this commit makes two improvements to clarify the
permission handling:

1. Changes the name to match better the contents.
2. Uses naming to differentiate the field inside the struct and
   the parameter passed to functions.

Cc: Sean Christopherson <sean.j.christpherson@intel.com>
Cc: Shay Katz-zamir <shay.katz-zamir@intel.com>
Cc: Serge Ayoun <serge.ayoun@intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/ioctl.c |  2 +-
 arch/x86/kernel/cpu/sgx/encl.c         | 14 +++++++-------
 arch/x86/kernel/cpu/sgx/encl.h         |  4 ++--
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 2415dcb20a6e..b61e06daad6e 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -261,7 +261,7 @@ static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
 	encl_page->encl = encl;
 
 	/* Calculate maximum of the VM flags for the page. */
-	encl_page->vm_prot_bits = calc_vm_prot_bits(prot, 0);
+	encl_page->max_vm_flags = calc_vm_prot_bits(prot, 0);
 
 	ret = radix_tree_insert(&encl->page_tree, PFN_DOWN(encl_page->desc),
 				encl_page);
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index a20d498e9dcd..890eacb45a80 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -292,25 +292,25 @@ static unsigned int sgx_vma_fault(struct vm_fault *vmf)
  * @encl:		an enclave
  * @start:		lower bound of the address range, inclusive
  * @end:		upper bound of the address range, exclusive
- * @vm_prot_bits:	requested protections of the address range
+ * @vm_flags:		requested VM flags for the address range
  *
  * Iterate through the enclave pages contained within [@start, @end) to verify
- * the permissions requested by @vm_prot_bits do not exceed that of any enclave
- * page to be mapped.  Page addresses that do not have an associated enclave
- * page are interpreted to zero permissions.
+ * the the VM flags do not exceed that of any enclave page to be mapped. Page
+ * addresses that do not have an associated enclave page are interpreted to zero
+ * permissions.
  *
  * Return:
  *   0 on success,
  *   -EACCES if VMA permissions exceed enclave page permissions
  */
 int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
-		     unsigned long end, unsigned long vm_prot_bits)
+		     unsigned long end, unsigned long vm_flags)
 {
 	unsigned long idx, idx_start, idx_end;
 	struct sgx_encl_page *page;
 
 	/* PROT_NONE always succeeds. */
-	if (!vm_prot_bits)
+	if (!vm_flags)
 		return 0;
 
 	idx_start = PFN_DOWN(start);
@@ -321,7 +321,7 @@ int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
 		page = radix_tree_lookup(&encl->page_tree, idx);
 		mutex_unlock(&encl->lock);
 
-		if (!page || (~page->vm_prot_bits & vm_prot_bits))
+		if (!page || (~page->max_vm_flags & vm_flags))
 			return -EACCES;
 	}
 
diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
index d3a1687ed84c..0e28b784a8c5 100644
--- a/arch/x86/kernel/cpu/sgx/encl.h
+++ b/arch/x86/kernel/cpu/sgx/encl.h
@@ -44,7 +44,7 @@ enum sgx_encl_page_desc {
 
 struct sgx_encl_page {
 	unsigned long desc;
-	unsigned long vm_prot_bits;
+	unsigned long max_vm_flags;
 	struct sgx_epc_page *epc_page;
 	struct sgx_va_page *va_page;
 	struct sgx_encl *encl;
@@ -134,6 +134,6 @@ void sgx_free_va_slot(struct sgx_va_page *va_page, unsigned int offset);
 bool sgx_va_page_full(struct sgx_va_page *va_page);
 
 int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
-		     unsigned long end, unsigned long vm_prot_bits);
+		     unsigned long end, unsigned long vm_flags);
 
 #endif /* _X86_ENCL_H */
-- 
2.20.1

Re: [PATCH 5/5] x86/sgx: Rename vm_prot_bits as max_vm_flags

[Sean Christopherson]

On Mon, Aug 19, 2019 at 06:25:44PM +0300, Jarkko Sakkinen wrote:
> vm_prot_bits is very bad and misleading name for the field in struct
> sgx_encl_page. What the field contains exactly is not @prot of
> mprotect() but the *maximum* VM flags for the VMA that contains the
> given enclave page.
> 
> Thus, the only viable name for the field is max_vm_flags. In functions
> that also pass VM flags the parameter name is renamed from vm_prot_bits
> to vm_flags.

Why not max_vm_prot_bits?  'vm_flags' implies the field contains all
manner of VM_FLAGS.  The 'vm_prot_bits' name was derived from
calc_vm_prot_bits() to provide this differentiation, especially since
there's also a calc_vm_flag_bits(),

> To summarize, this commit makes two improvements to clarify the
> permission handling:
> 
> 1. Changes the name to match better the contents.
> 2. Uses naming to differentiate the field inside the struct and
>    the parameter passed to functions.
> 
> Cc: Sean Christopherson <sean.j.christpherson@intel.com>
> Cc: Shay Katz-zamir <shay.katz-zamir@intel.com>
> Cc: Serge Ayoun <serge.ayoun@intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/driver/ioctl.c |  2 +-
>  arch/x86/kernel/cpu/sgx/encl.c         | 14 +++++++-------
>  arch/x86/kernel/cpu/sgx/encl.h         |  4 ++--
>  3 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index 2415dcb20a6e..b61e06daad6e 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -261,7 +261,7 @@ static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
>  	encl_page->encl = encl;
>  
>  	/* Calculate maximum of the VM flags for the page. */
> -	encl_page->vm_prot_bits = calc_vm_prot_bits(prot, 0);
> +	encl_page->max_vm_flags = calc_vm_prot_bits(prot, 0);
>  
>  	ret = radix_tree_insert(&encl->page_tree, PFN_DOWN(encl_page->desc),
>  				encl_page);
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index a20d498e9dcd..890eacb45a80 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -292,25 +292,25 @@ static unsigned int sgx_vma_fault(struct vm_fault *vmf)
>   * @encl:		an enclave
>   * @start:		lower bound of the address range, inclusive
>   * @end:		upper bound of the address range, exclusive
> - * @vm_prot_bits:	requested protections of the address range
> + * @vm_flags:		requested VM flags for the address range
>   *
>   * Iterate through the enclave pages contained within [@start, @end) to verify
> - * the permissions requested by @vm_prot_bits do not exceed that of any enclave
> - * page to be mapped.  Page addresses that do not have an associated enclave
> - * page are interpreted to zero permissions.
> + * the the VM flags do not exceed that of any enclave page to be mapped. Page
> + * addresses that do not have an associated enclave page are interpreted to zero
> + * permissions.
>   *
>   * Return:
>   *   0 on success,
>   *   -EACCES if VMA permissions exceed enclave page permissions
>   */
>  int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
> -		     unsigned long end, unsigned long vm_prot_bits)
> +		     unsigned long end, unsigned long vm_flags)

If 'vm_prot_bits' is distasteful, 'prot' would be preferrable as its used
throughout the kernel for variable that hold only the prot bits.

>  {
>  	unsigned long idx, idx_start, idx_end;
>  	struct sgx_encl_page *page;
>  
>  	/* PROT_NONE always succeeds. */
> -	if (!vm_prot_bits)
> +	if (!vm_flags)
>  		return 0;
>  
>  	idx_start = PFN_DOWN(start);
> @@ -321,7 +321,7 @@ int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
>  		page = radix_tree_lookup(&encl->page_tree, idx);
>  		mutex_unlock(&encl->lock);
>  
> -		if (!page || (~page->vm_prot_bits & vm_prot_bits))
> +		if (!page || (~page->max_vm_flags & vm_flags))
>  			return -EACCES;
>  	}
>  
> diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
> index d3a1687ed84c..0e28b784a8c5 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.h
> +++ b/arch/x86/kernel/cpu/sgx/encl.h
> @@ -44,7 +44,7 @@ enum sgx_encl_page_desc {
>  
>  struct sgx_encl_page {
>  	unsigned long desc;
> -	unsigned long vm_prot_bits;
> +	unsigned long max_vm_flags;
>  	struct sgx_epc_page *epc_page;
>  	struct sgx_va_page *va_page;
>  	struct sgx_encl *encl;
> @@ -134,6 +134,6 @@ void sgx_free_va_slot(struct sgx_va_page *va_page, unsigned int offset);
>  bool sgx_va_page_full(struct sgx_va_page *va_page);
>  
>  int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
> -		     unsigned long end, unsigned long vm_prot_bits);
> +		     unsigned long end, unsigned long vm_flags);
>  
>  #endif /* _X86_ENCL_H */
> -- 
> 2.20.1
> 

Re: [PATCH 5/5] x86/sgx: Rename vm_prot_bits as max_vm_flags

[Jarkko Sakkinen]

On Wed, 2019-08-21 at 21:00 -0700, Sean Christopherson wrote:
> On Mon, Aug 19, 2019 at 06:25:44PM +0300, Jarkko Sakkinen wrote:
> > vm_prot_bits is very bad and misleading name for the field in struct
> > sgx_encl_page. What the field contains exactly is not @prot of
> > mprotect() but the *maximum* VM flags for the VMA that contains the
> > given enclave page.
> > 
> > Thus, the only viable name for the field is max_vm_flags. In functions
> > that also pass VM flags the parameter name is renamed from vm_prot_bits
> > to vm_flags.
> 
> Why not max_vm_prot_bits?  'vm_flags' implies the field contains all
> manner of VM_FLAGS.  The 'vm_prot_bits' name was derived from
> calc_vm_prot_bits() to provide this differentiation, especially since
> there's also a calc_vm_flag_bits(),

Squashed with vm_max_prot_bits.

/Jarkko