| Message ID | 20200918130226.39530-1-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Refine rollback in SGX_IOC_ENCLAVE_ADD_PAGE | expand |
On Fri, 18 Sep 2020 08:02:26 -0500, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote: > Revert back 'count' to struct sgx_enclave_add_pages as in most of the > cases enclave can be recovered. Refine the documentation to better > describe the enclave is persisted. > > Move the check for -EIO from sgx_encl_add_page() to > sgx_ioc_enclave_pages() to make it more visible. It was quite > hidden over there. > > Cc: Sean Christopherson <sean.j.christopherson@intel.com> > Cc: Haitao Huang <haitao.huang@linux.intel.com> > Cc: Borislav Petkov <bp@alien8.de> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > arch/x86/include/uapi/asm/sgx.h | 12 +++++++----- > arch/x86/kernel/cpu/sgx/ioctl.c | 29 ++++++++++++++--------------- > 2 files changed, 21 insertions(+), 20 deletions(-) > > diff --git a/arch/x86/include/uapi/asm/sgx.h > b/arch/x86/include/uapi/asm/sgx.h > index 1564d7f88597..ec5157b2ff50 100644 > --- a/arch/x86/include/uapi/asm/sgx.h > +++ b/arch/x86/include/uapi/asm/sgx.h > @@ -45,13 +45,15 @@ struct sgx_enclave_create { > * @length: length of the data (multiple of the page size) > * @secinfo: address for the SECINFO data > * @flags: page control flags > + * @count: number of bytes added (multiple of the page size) > */ > struct sgx_enclave_add_pages { > - __u64 src; > - __u64 offset; > - __u64 length; > - __u64 secinfo; > - __u64 flags; > + __u64 src; > + __u64 offset; > + __u64 length; > + __u64 secinfo; > + __u64 flags; > + __u64 count; > }; > /** > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c > b/arch/x86/kernel/cpu/sgx/ioctl.c > index 30de66f4247b..d10179b47daa 100644 > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > @@ -449,16 +449,6 @@ static int sgx_encl_add_page(struct sgx_encl *encl, > unsigned long src, > sgx_free_epc_page(epc_page); > kfree(encl_page); > - /* > - * Destroy enclave on ENCLS failure as this means that EPC has been > - * invalidated. > - */ > - if (ret == -EIO) { > - mutex_lock(&encl->lock); > - sgx_encl_destroy(encl); > - mutex_unlock(&encl->lock); > - } > - > return ret; > } > @@ -487,12 +477,15 @@ static int sgx_encl_add_page(struct sgx_encl > *encl, unsigned long src, > * > * If ENCLS opcode fails, that effectively means that EPC has been > invalidated. > * When this happens the enclave is destroyed and -EIO is returned to > the > - * caller. > + * caller. In this situation the function destroys the enclave as it > cannot > + * be recovered. > * > * Return: > * length of the data processed on success, > * -EACCES if an executable source page is located in a noexec > partition, > - * -EIO if either ENCLS[EADD] or ENCLS[EEXTEND] fails > + * -ENOMEM if the system is out of EPC pages, > + * -EINTR if the call was interrupted before any data was processed, > + * -EIO if either ENCLS[EADD] or ENCLS[EEXTEND] fails, > * -errno otherwise > */ > static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void > __user *arg) > @@ -549,10 +542,16 @@ static long sgx_ioc_enclave_add_pages(struct > sgx_encl *encl, void __user *arg) > break; > } > - if (ret) > - return ret; > + addp.count = c; > + > + /* On EADD or EEXTEND failure, destroy the enclave. */ > + if (ret == -EIO) { > + mutex_lock(&encl->lock); > + sgx_encl_destroy(encl); > + mutex_unlock(&encl->lock); > + } > - return c; missing copy_to_user for addp.count? > + return ret; > } > static int __sgx_get_key_hash(struct crypto_shash *tfm, const void > *modulus,
On Mon, Sep 28, 2020 at 04:24:23PM -0500, Haitao Huang wrote: > On Fri, 18 Sep 2020 08:02:26 -0500, Jarkko Sakkinen > <jarkko.sakkinen@linux.intel.com> wrote: > > > Revert back 'count' to struct sgx_enclave_add_pages as in most of the > > cases enclave can be recovered. Refine the documentation to better > > describe the enclave is persisted. > > > > Move the check for -EIO from sgx_encl_add_page() to > > sgx_ioc_enclave_pages() to make it more visible. It was quite > > hidden over there. > > > > Cc: Sean Christopherson <sean.j.christopherson@intel.com> > > Cc: Haitao Huang <haitao.huang@linux.intel.com> > > Cc: Borislav Petkov <bp@alien8.de> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > --- > > arch/x86/include/uapi/asm/sgx.h | 12 +++++++----- > > arch/x86/kernel/cpu/sgx/ioctl.c | 29 ++++++++++++++--------------- > > 2 files changed, 21 insertions(+), 20 deletions(-) > > > > diff --git a/arch/x86/include/uapi/asm/sgx.h > > b/arch/x86/include/uapi/asm/sgx.h > > index 1564d7f88597..ec5157b2ff50 100644 > > --- a/arch/x86/include/uapi/asm/sgx.h > > +++ b/arch/x86/include/uapi/asm/sgx.h > > @@ -45,13 +45,15 @@ struct sgx_enclave_create { > > * @length: length of the data (multiple of the page size) > > * @secinfo: address for the SECINFO data > > * @flags: page control flags > > + * @count: number of bytes added (multiple of the page size) > > */ > > struct sgx_enclave_add_pages { > > - __u64 src; > > - __u64 offset; > > - __u64 length; > > - __u64 secinfo; > > - __u64 flags; > > + __u64 src; > > + __u64 offset; > > + __u64 length; > > + __u64 secinfo; > > + __u64 flags; > > + __u64 count; > > }; > > /** > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c > > b/arch/x86/kernel/cpu/sgx/ioctl.c > > index 30de66f4247b..d10179b47daa 100644 > > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > > @@ -449,16 +449,6 @@ static int sgx_encl_add_page(struct sgx_encl *encl, > > unsigned long src, > > sgx_free_epc_page(epc_page); > > kfree(encl_page); > > - /* > > - * Destroy enclave on ENCLS failure as this means that EPC has been > > - * invalidated. > > - */ > > - if (ret == -EIO) { > > - mutex_lock(&encl->lock); > > - sgx_encl_destroy(encl); > > - mutex_unlock(&encl->lock); > > - } > > - > > return ret; > > } > > @@ -487,12 +477,15 @@ static int sgx_encl_add_page(struct sgx_encl > > *encl, unsigned long src, > > * > > * If ENCLS opcode fails, that effectively means that EPC has been > > invalidated. > > * When this happens the enclave is destroyed and -EIO is returned to > > the > > - * caller. > > + * caller. In this situation the function destroys the enclave as it > > cannot > > + * be recovered. > > * > > * Return: > > * length of the data processed on success, > > * -EACCES if an executable source page is located in a noexec > > partition, > > - * -EIO if either ENCLS[EADD] or ENCLS[EEXTEND] fails > > + * -ENOMEM if the system is out of EPC pages, > > + * -EINTR if the call was interrupted before any data was processed, > > + * -EIO if either ENCLS[EADD] or ENCLS[EEXTEND] fails, > > * -errno otherwise > > */ > > static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void > > __user *arg) > > @@ -549,10 +542,16 @@ static long sgx_ioc_enclave_add_pages(struct > > sgx_encl *encl, void __user *arg) > > break; > > } > > - if (ret) > > - return ret; > > + addp.count = c; > > + > > + /* On EADD or EEXTEND failure, destroy the enclave. */ > > + if (ret == -EIO) { > > + mutex_lock(&encl->lock); > > + sgx_encl_destroy(encl); > > + mutex_unlock(&encl->lock); > > + } > > - return c; > > missing copy_to_user for addp.count? This somewhat deprecated patch. It's now all fixed in my master branch. /Jarkko
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h index 1564d7f88597..ec5157b2ff50 100644 --- a/arch/x86/include/uapi/asm/sgx.h +++ b/arch/x86/include/uapi/asm/sgx.h @@ -45,13 +45,15 @@ struct sgx_enclave_create { * @length: length of the data (multiple of the page size) * @secinfo: address for the SECINFO data * @flags: page control flags + * @count: number of bytes added (multiple of the page size) */ struct sgx_enclave_add_pages { - __u64 src; - __u64 offset; - __u64 length; - __u64 secinfo; - __u64 flags; + __u64 src; + __u64 offset; + __u64 length; + __u64 secinfo; + __u64 flags; + __u64 count; }; /** diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index 30de66f4247b..d10179b47daa 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -449,16 +449,6 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src, sgx_free_epc_page(epc_page); kfree(encl_page); - /* - * Destroy enclave on ENCLS failure as this means that EPC has been - * invalidated. - */ - if (ret == -EIO) { - mutex_lock(&encl->lock); - sgx_encl_destroy(encl); - mutex_unlock(&encl->lock); - } - return ret; } @@ -487,12 +477,15 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src, * * If ENCLS opcode fails, that effectively means that EPC has been invalidated. * When this happens the enclave is destroyed and -EIO is returned to the - * caller. + * caller. In this situation the function destroys the enclave as it cannot + * be recovered. * * Return: * length of the data processed on success, * -EACCES if an executable source page is located in a noexec partition, - * -EIO if either ENCLS[EADD] or ENCLS[EEXTEND] fails + * -ENOMEM if the system is out of EPC pages, + * -EINTR if the call was interrupted before any data was processed, + * -EIO if either ENCLS[EADD] or ENCLS[EEXTEND] fails, * -errno otherwise */ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg) @@ -549,10 +542,16 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg) break; } - if (ret) - return ret; + addp.count = c; + + /* On EADD or EEXTEND failure, destroy the enclave. */ + if (ret == -EIO) { + mutex_lock(&encl->lock); + sgx_encl_destroy(encl); + mutex_unlock(&encl->lock); + } - return c; + return ret; } static int __sgx_get_key_hash(struct crypto_shash *tfm, const void *modulus,
Revert back 'count' to struct sgx_enclave_add_pages as in most of the cases enclave can be recovered. Refine the documentation to better describe the enclave is persisted. Move the check for -EIO from sgx_encl_add_page() to sgx_ioc_enclave_pages() to make it more visible. It was quite hidden over there. Cc: Sean Christopherson <sean.j.christopherson@intel.com> Cc: Haitao Huang <haitao.huang@linux.intel.com> Cc: Borislav Petkov <bp@alien8.de> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- arch/x86/include/uapi/asm/sgx.h | 12 +++++++----- arch/x86/kernel/cpu/sgx/ioctl.c | 29 ++++++++++++++--------------- 2 files changed, 21 insertions(+), 20 deletions(-)