| Message ID | 20191009044241.3591-4-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Improve add pages ioctl | expand |
On Tue, Oct 08, 2019 at 09:42:37PM -0700, Sean Christopherson wrote: > Add a nr_pages param to the ioctl for adding pages to the enclave so > that userspace can batch multiple pages in a single syscall. Update the > offset, src and nr_pages params prior to returning to userspace so that > the caller has sufficient information to analyze failures and can easily > restart the ioctl when appropriate. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> Please provide a more robust API. Now you decrease the robustness. E.g. struct sgx_enclave_add_page_desc { __u64 offset; __u16 mrmask; __u8 reserved[6]; }; struct sgx_enclave_add_page { __u64 src; __u64 secinfo; __u64 nr_pages; __u64 pages; }; /Jarkko
On Tue, Oct 15, 2019 at 12:32:55AM +0300, Jarkko Sakkinen wrote: > On Tue, Oct 08, 2019 at 09:42:37PM -0700, Sean Christopherson wrote: > > Add a nr_pages param to the ioctl for adding pages to the enclave so > > that userspace can batch multiple pages in a single syscall. Update the > > offset, src and nr_pages params prior to returning to userspace so that > > the caller has sufficient information to analyze failures and can easily > > restart the ioctl when appropriate. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > Please provide a more robust API. Now you decrease the robustness. > > E.g. > > struct sgx_enclave_add_page_desc { > __u64 offset; > __u16 mrmask; > __u8 reserved[6]; > }; > > struct sgx_enclave_add_page { > __u64 src; > __u64 secinfo; > __u64 nr_pages; > __u64 pages; > }; If you want to decrease robustness, this would need to be taken as part of v23 review. It is too big design change to managed like this. I'm not opionated here. This is just wrong order of doing things. /Jarkko
On Tue, Oct 15, 2019 at 12:35:46AM +0300, Jarkko Sakkinen wrote: > On Tue, Oct 15, 2019 at 12:32:55AM +0300, Jarkko Sakkinen wrote: > > On Tue, Oct 08, 2019 at 09:42:37PM -0700, Sean Christopherson wrote: > > > Add a nr_pages param to the ioctl for adding pages to the enclave so > > > that userspace can batch multiple pages in a single syscall. Update the > > > offset, src and nr_pages params prior to returning to userspace so that > > > the caller has sufficient information to analyze failures and can easily > > > restart the ioctl when appropriate. > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > > Please provide a more robust API. Now you decrease the robustness. > > > > E.g. > > > > struct sgx_enclave_add_page_desc { > > __u64 offset; > > __u16 mrmask; > > __u8 reserved[6]; > > }; > > > > struct sgx_enclave_add_page { > > __u64 src; > > __u64 secinfo; > > __u64 nr_pages; > > __u64 pages; > > }; > > If you want to decrease robustness, this would need to be taken as part > of v23 review. It is too big design change to managed like this. I'm > not opionated here. This is just wrong order of doing things. I don't mind taking this to v23 review, but what do you mean by robustness in this context?
On Mon, Oct 14, 2019 at 04:31:28PM -0700, Sean Christopherson wrote: > I don't mind taking this to v23 review, but what do you mean by robustness > in this context? I think I kind of got this together API-wise: #define SGX_ENCLAVE_ADD_PAGES_MEASURE 1 struct sgx_enclave_add_pages { __u64 src; __u64 offset; __u64 length; __u64 secinfo; }; Length can be anything as long as low 8 bits are zero. The area defined by offset and length is measured when SGX_ENCLAVE_ADD_PAGES_MEASURE is set to 1. I think this is the most sane API so far and does fulfill Jethro's concerns why he originally wanted mrmask. I think this what most users would find the most intuitive API. Jethro, do you think you could live with this? /Jarkko
On Wed, Oct 16, 2019 at 01:17:23PM +0300, Jarkko Sakkinen wrote: > On Mon, Oct 14, 2019 at 04:31:28PM -0700, Sean Christopherson wrote: > > I don't mind taking this to v23 review, but what do you mean by robustness > > in this context? > > I think I kind of got this together API-wise: > > #define SGX_ENCLAVE_ADD_PAGES_MEASURE 1 > > struct sgx_enclave_add_pages { > __u64 src; > __u64 offset; > __u64 length; > __u64 secinfo; > }; > > Length can be anything as long as low 8 bits are zero. The area > defined by offset and length is measured when > SGX_ENCLAVE_ADD_PAGES_MEASURE is set to 1. > > I think this is the most sane API so far and does fulfill Jethro's > concerns why he originally wanted mrmask. I think this what most > users would find the most intuitive API. > > Jethro, do you think you could live with this? This just a version of Sean's API but with sane defaults for mrmask. /Jarkko
On Wed, Oct 16, 2019 at 01:19:56PM +0300, Jarkko Sakkinen wrote: > On Wed, Oct 16, 2019 at 01:17:23PM +0300, Jarkko Sakkinen wrote: > > On Mon, Oct 14, 2019 at 04:31:28PM -0700, Sean Christopherson wrote: > > > I don't mind taking this to v23 review, but what do you mean by robustness > > > in this context? > > > > I think I kind of got this together API-wise: > > > > #define SGX_ENCLAVE_ADD_PAGES_MEASURE 1 > > > > struct sgx_enclave_add_pages { > > __u64 src; > > __u64 offset; > > __u64 length; > > __u64 secinfo; > > }; > > > > Length can be anything as long as low 8 bits are zero. The area > > defined by offset and length is measured when > > SGX_ENCLAVE_ADD_PAGES_MEASURE is set to 1. > > > > I think this is the most sane API so far and does fulfill Jethro's > > concerns why he originally wanted mrmask. I think this what most > > users would find the most intuitive API. > > > > Jethro, do you think you could live with this? > > This just a version of Sean's API but with sane defaults for mrmask. Now that mrmask is rendered out the general idea of defining continuous regions rather than scattered arrays of descriptors is superior. And it is also obvious that a single page ioctl would be ugly glitch even if it wouldn't cause harm. /Jarkko
On Wed, Oct 16, 2019 at 01:29:12PM +0300, Jarkko Sakkinen wrote: > On Wed, Oct 16, 2019 at 01:19:56PM +0300, Jarkko Sakkinen wrote: > > On Wed, Oct 16, 2019 at 01:17:23PM +0300, Jarkko Sakkinen wrote: > > > On Mon, Oct 14, 2019 at 04:31:28PM -0700, Sean Christopherson wrote: > > > > I don't mind taking this to v23 review, but what do you mean by robustness > > > > in this context? > > > > > > I think I kind of got this together API-wise: > > > > > > #define SGX_ENCLAVE_ADD_PAGES_MEASURE 1 > > > > > > struct sgx_enclave_add_pages { > > > __u64 src; > > > __u64 offset; > > > __u64 length; > > > __u64 secinfo; > > > }; Offset would be split as 48:8:8 aka PAGE_INDEX:MRSTART:FLAGS Length would be split as 48:8:8 aka NR_PAGES:MREND:0 Forgot from my earlier description that measurement starting point would better to be also after the page start but with this bit presentation everything can be cleanly described. /Jarkko
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h index 67583b046af1..84734229d8dd 100644 --- a/arch/x86/include/uapi/asm/sgx.h +++ b/arch/x86/include/uapi/asm/sgx.h @@ -12,8 +12,8 @@ #define SGX_IOC_ENCLAVE_CREATE \ _IOW(SGX_MAGIC, 0x00, struct sgx_enclave_create) -#define SGX_IOC_ENCLAVE_ADD_PAGE \ - _IOW(SGX_MAGIC, 0x01, struct sgx_enclave_add_page) +#define SGX_IOC_ENCLAVE_ADD_PAGES \ + _IOWR(SGX_MAGIC, 0x01, struct sgx_enclave_add_pages) #define SGX_IOC_ENCLAVE_INIT \ _IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init) #define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \ @@ -29,17 +29,19 @@ struct sgx_enclave_create { }; /** - * struct sgx_enclave_add_page - parameter structure for the - * %SGX_IOC_ENCLAVE_ADD_PAGE ioctl - * @offset: page offset within the enclave - * @src: address for the page data + * struct sgx_enclave_add_pages - parameter structure for the + * %SGX_IOC_ENCLAVE_ADD_PAGE ioctl + * @offset: starting page offset within the ELRANGE + * @src: start address for the page data + * @nr_pages: number of pages to add to enclave * @secinfo: address for the SECINFO data * @mrmask: bitmask for the measured 256 byte chunks * @reserved: reserved for future use */ -struct sgx_enclave_add_page { +struct sgx_enclave_add_pages { __u64 offset; __u64 src; + __u64 nr_pages; __u64 secinfo; __u16 mrmask; __u8 reserved[6]; diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index f407dd35f9e3..4597dd8f5c91 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -360,7 +360,7 @@ static int __sgx_encl_extend(struct sgx_encl *encl, } static int sgx_encl_add_page(struct sgx_encl *encl, - struct sgx_enclave_add_page *addp, + struct sgx_enclave_add_pages *addp, struct sgx_secinfo *secinfo) { struct sgx_encl_page *encl_page; @@ -443,14 +443,19 @@ static int sgx_encl_add_page(struct sgx_encl *encl, } /** - * sgx_ioc_enclave_add_page() - The handler for %SGX_IOC_ENCLAVE_ADD_PAGE - * @filep: open file to /dev/sgx - * @arg: a user pointer to a struct sgx_enclave_add_page instance + * sgx_ioc_enclave_add_pages() - The handler for %SGX_IOC_ENCLAVE_ADD_PAGES + * @encl: pointer to an enclave instance (via ioctl() file pointer) + * @arg: a user pointer to a struct sgx_enclave_add_pages instance * - * Add (EADD) a page to an uninitialized enclave, and optionally extend - * (EEXTEND) the measurement with the contents of the page. A SECINFO for a TCS - * is required to always contain zero permissions because CPU silently zeros - * them. Allowing anything else would cause a mismatch in the measurement. + * Add (EADD) one or more pages to an uninitialized enclave, and optionally + * extend (EEXTEND) the measurement with the contents of the page. The range of + * pages must be virtually contiguous. The SECINFO and measurement mask are + * applied to all pages, i.e. pages with different properties must be added in + * separate calls. + * + * A SECINFO for a TCS is required to always contain zero permissions because + * CPU silently zeros them. Allowing anything else would cause a mismatch in + * the measurement. * * mmap()'s protection bits are capped by the page permissions. For each page * address, the maximum protection bits are computed with the following @@ -467,17 +472,25 @@ static int sgx_encl_add_page(struct sgx_encl *encl, * permissions. In effect, this allows mmap() with PROT_NONE to be used to seek * an address range for the enclave that can be then populated into SECS. * + * @arg->addr, @arg->src and @arg->nr_pages are adjusted to reflect the + * remaining pages that need to be added to the enclave, e.g. userspace can + * re-invoke SGX_IOC_ENCLAVE_ADD_PAGES using the same struct in response to an + * ERESTARTSYS error. + * * Return: * 0 on success, - * -EINVAL if the SECINFO contains invalid data, - * -EACCES if the source page is located in a noexec partition, + * -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, * -errno otherwise + */ -static long sgx_ioc_enclave_add_page(struct sgx_encl *encl, void __user *arg) +static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg) { - struct sgx_enclave_add_page addp; + struct sgx_enclave_add_pages addp; struct sgx_secinfo secinfo; + int ret; if (!(atomic_read(&encl->flags) & SGX_ENCL_CREATED)) return -EINVAL; @@ -489,7 +502,10 @@ static long sgx_ioc_enclave_add_page(struct sgx_encl *encl, void __user *arg) !IS_ALIGNED(addp.src, PAGE_SIZE)) return -EINVAL; - if (addp.offset >= encl->size) + if (!addp.nr_pages) + return -EINVAL; + + if (addp.offset + ((addp.nr_pages - 1) * PAGE_SIZE) >= encl->size) return -EINVAL; if (copy_from_user(&secinfo, (void __user *)addp.secinfo, @@ -499,7 +515,27 @@ static long sgx_ioc_enclave_add_page(struct sgx_encl *encl, void __user *arg) if (sgx_validate_secinfo(&secinfo)) return -EINVAL; - return sgx_encl_add_page(encl, &addp, &secinfo); + for ( ; addp.nr_pages > 0; addp.nr_pages--) { + if (signal_pending(current)) { + ret = -ERESTARTSYS; + break; + } + + if (need_resched()) + cond_resched(); + + ret = sgx_encl_add_page(encl, &addp, &secinfo); + if (ret) + break; + + addp.offset += PAGE_SIZE; + addp.src += PAGE_SIZE; + } + + if (copy_to_user(arg, &addp, sizeof(addp))) + return -EFAULT; + + return ret; } static int __sgx_get_key_hash(struct crypto_shash *tfm, const void *modulus, @@ -702,8 +738,8 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) case SGX_IOC_ENCLAVE_CREATE: ret = sgx_ioc_enclave_create(encl, (void __user *)arg); break; - case SGX_IOC_ENCLAVE_ADD_PAGE: - ret = sgx_ioc_enclave_add_page(encl, (void __user *)arg); + case SGX_IOC_ENCLAVE_ADD_PAGES: + ret = sgx_ioc_enclave_add_pages(encl, (void __user *)arg); break; case SGX_IOC_ENCLAVE_INIT: ret = sgx_ioc_enclave_init(encl, (void __user *)arg);
Add a nr_pages param to the ioctl for adding pages to the enclave so that userspace can batch multiple pages in a single syscall. Update the offset, src and nr_pages params prior to returning to userspace so that the caller has sufficient information to analyze failures and can easily restart the ioctl when appropriate. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/include/uapi/asm/sgx.h | 16 ++++---- arch/x86/kernel/cpu/sgx/ioctl.c | 68 +++++++++++++++++++++++++-------- 2 files changed, 61 insertions(+), 23 deletions(-)