2021-01-20 04:06:20

by Tianjia Zhang

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

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.

Reported-by: Jia Zhang <[email protected]>
Suggested-by: Sean Christopherson <[email protected]>
Signed-off-by: Tianjia Zhang <[email protected]>
---
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;
+
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

2021-01-20 16:24:51

by Sean Christopherson

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 <[email protected]>
> Suggested-by: Sean Christopherson <[email protected]>
> Signed-off-by: Tianjia Zhang <[email protected]>
> ---
> 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 <[email protected]>

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