Re: [PATCH v2] x86/sgx: Allows ioctl PROVISION to execute before CREATE

[Sean Christopherson]

On Wed, Jan 20, 2021, Tianjia Zhang wrote:
> In function sgx_encl_create(), the logic of directly assigning
> value to attributes_mask determines that the call to
> SGX_IOC_ENCLAVE_PROVISION must be after the command of
> SGX_IOC_ENCLAVE_CREATE. If move this assignment statement to
> function sgx_open(), the PROVISION command can be executed
> earlier and more flexibly.

Use imperative mood to describe what you're doing, e.g. the "if" leaves the
reader wonder whether it's a suggestion or what the patch is actually doing.

E.g. something like

  Move the initialization of an enclave's allowed attributes_mask to
  sgx_open() to allow the ENCLAVE_PROVISION ioctl() to be invoked before
  ENCLAVE_CREATE.

> Reported-by: Jia Zhang <zhang.jia@xxxxxxxxxxxxxxxxx>
> Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> Signed-off-by: Tianjia Zhang <tianjia.zhang@xxxxxxxxxxxxxxxxx>
> ---
>  arch/x86/kernel/cpu/sgx/driver.c | 3 +++
>  arch/x86/kernel/cpu/sgx/ioctl.c  | 1 -
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
> index f2eac41bb4ff..8766580194ae 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.c
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -36,6 +36,9 @@ static int sgx_open(struct inode *inode, struct file *file)
>  		return ret;
>  	}
>  
> +	encl->attributes_mask = SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT |
> +				SGX_ATTR_KSS;

Probably don't need a newline before SGX_ATTR_KSS.  In fact, I wouldn't be
surprised if Boris explicitly requested letting the original code go past 80
chars to improve readability.  Either way, with a cleaned up changelog:

Reviewed-by: Sean Christopherson <seanjc@xxxxxxxxxx> 

> +
>  	file->private_data = encl;
>  
>  	return 0;
> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> index 90a5caf76939..1c6ecf9fbeff 100644
> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -109,7 +109,6 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
>  	encl->base = secs->base;
>  	encl->size = secs->size;
>  	encl->attributes = secs->attributes;
> -	encl->attributes_mask = SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT | SGX_ATTR_KSS;
>  
>  	/* Set only after completion, as encl->lock has not been taken. */
>  	set_bit(SGX_ENCL_CREATED, &encl->flags);
> -- 
> 2.19.1.3.ge56e4f7
>