| Message ID | 20191105112056.21452-1-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for,v24,v2,1/4] x86/sgx: Destroy enclave if EADD fails | expand |
On Tue, Nov 05, 2019 at 01:20:53PM +0200, Jarkko Sakkinen wrote: > __sgx_encl_add_page() can only fail in the case of EPCM conflict at least > in non-artificial situations. Also, it consistent semantics in rollback is > something to pursue for. Thus, destroy enclave when the EADD fails as we do > when EEXTEND fails already. I still don't understand the motiviation for this change, EADD can fault and fail for reasons that are purely under userspace control. Yes, it's all but guaranteed to be a userspace bug, but I can't think of another instance in the kernel where the reaction to what is effectively an invalid param is to torch the whole thing. EEXTEND is special cased because the kernel doesn't have any other sane choice.
On Tue, Nov 05, 2019 at 02:58:33PM -0800, Sean Christopherson wrote: > On Tue, Nov 05, 2019 at 01:20:53PM +0200, Jarkko Sakkinen wrote: > > __sgx_encl_add_page() can only fail in the case of EPCM conflict at least > > in non-artificial situations. Also, it consistent semantics in rollback is > > something to pursue for. Thus, destroy enclave when the EADD fails as we do > > when EEXTEND fails already. > > I still don't understand the motiviation for this change, EADD can fault > and fail for reasons that are purely under userspace control. Yes, it's > all but guaranteed to be a userspace bug, but I can't think of another > instance in the kernel where the reaction to what is effectively an invalid > param is to torch the whole thing. EEXTEND is special cased because the > kernel doesn't have any other sane choice. -EIO should be returned in both cases so that caller has a way to determine if the ENCLS operations failed. They sum into a transaction that is why the rollback must be the same for any sane semantics. If the caller wants to purposely cause that, it is caller choice. And we don't want to purposely support completely undeterministic behaviour when there is no backwards compatibility to maintain. /Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index d53aee5a64c1..289af607f634 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -338,7 +338,7 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, kunmap_atomic((void *)pginfo.contents); put_page(src_page); - return ret ? -EFAULT : 0; + return ret ? -EIO : 0; } static int __sgx_encl_extend(struct sgx_encl *encl, @@ -353,7 +353,7 @@ static int __sgx_encl_extend(struct sgx_encl *encl, if (ret) { if (encls_failed(ret)) ENCLS_WARN(ret, "EEXTEND"); - return -EFAULT; + return -EIO; } } @@ -413,8 +413,10 @@ static int sgx_encl_add_page(struct sgx_encl *encl, ret = __sgx_encl_add_page(encl, encl_page, epc_page, secinfo, addp->src); - if (ret) + if (ret) { + sgx_encl_destroy(encl); goto err_out; + } /* * Complete the "add" before doing the "extend" so that the "add" @@ -498,10 +500,9 @@ static int sgx_encl_add_page(struct sgx_encl *encl, * * Return: * 0 on success, - * -EINVAL if any input param or the SECINFO contains invalid data, * -EACCES if an executable source page is located in a noexec partition, - * -ENOMEM if any memory allocation, including EPC, fails, - * -ERESTARTSYS if a pending signal is recognized + * -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) {
__sgx_encl_add_page() can only fail in the case of EPCM conflict at least in non-artificial situations. Also, it consistent semantics in rollback is something to pursue for. Thus, destroy enclave when the EADD fails as we do when EEXTEND fails already. In the cases it is sane to return -EIO. From this the caller can deduce the failure and knows that the enclave was destroyed. The previous -EFAULT could happen in numerous situations. Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- arch/x86/kernel/cpu/sgx/ioctl.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)