[3/5] x86/sgx: Make sgx_validate_secinfo() more readable
diff mbox series

Message ID 20190819152544.7296-4-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
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(-)

Comments

Sean Christopherson Aug. 22, 2019, 3:48 a.m. UTC | #1
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
>
Ayoun, Serge Aug. 22, 2019, 10:39 a.m. UTC | #2
> 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.
Jarkko Sakkinen Aug. 22, 2019, 4:26 p.m. UTC | #3
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
Jarkko Sakkinen Aug. 22, 2019, 4:45 p.m. UTC | #4
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
diff mbox series

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