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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 20 Jan 2021 08:20:25 -0800
From:   Sean Christopherson <seanjc@...gle.com>
To:     Tianjia Zhang <tianjia.zhang@...ux.alibaba.com>
Cc:     Jarkko Sakkinen <jarkko@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        linux-sgx@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jia Zhang <zhang.jia@...ux.alibaba.com>
Subject: Re: [PATCH v2] x86/sgx: Allows ioctl PROVISION to execute before
 CREATE

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@...ux.alibaba.com>
> Suggested-by: Sean Christopherson <seanjc@...gle.com>
> Signed-off-by: Tianjia Zhang <tianjia.zhang@...ux.alibaba.com>
> ---
>  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@...gle.com> 

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