| Message ID | 20190531233159.30992-4-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | security: x86/sgx: SGX vs. LSM | expand |
> From: Christopherson, Sean J > Sent: Friday, May 31, 2019 4:32 PM > > +/** > + * sgx_ioc_enclave_add_pages - handler for %SGX_IOC_ENCLAVE_ADD_PAGES > + * > + * @filep: open file to /dev/sgx > + * @cmd: the command value > + * @arg: pointer to an &sgx_enclave_add_page instance > + * > + * Add a range of pages to an uninitialized enclave (EADD), and > +optionally > + * extend the enclave's measurement with the contents of the page (EEXTEND). > + * The range of pages must be virtually contiguous. The SECINFO and > + * measurement maskare applied to all pages, i.e. pages with different > + * properties must be added in separate calls. > + * > + * EADD and EEXTEND are done asynchronously via worker threads. A > +successful > + * sgx_ioc_enclave_add_page() only indicates the pages have been added > +to the > + * work queue, it does not guarantee adding the pages to the enclave > +will > + * succeed. > + * > + * Return: > + * 0 on success, > + * -errno otherwise > + */ > +static long sgx_ioc_enclave_add_pages(struct file *filep, unsigned int cmd, > + unsigned long arg) > +{ > + struct sgx_enclave_add_pages *addp = (void *)arg; > + struct sgx_encl *encl = filep->private_data; > + struct sgx_secinfo secinfo; > + unsigned int i; > + int ret; > + > + if (copy_from_user(&secinfo, (void __user *)addp->secinfo, > + sizeof(secinfo))) > + return -EFAULT; > + > + for (i = 0, ret = 0; i < addp->nr_pages && !ret; i++) { > + if (signal_pending(current)) > + return -ERESTARTSYS; If interrupted, how would user mode code know how many pages have been EADD'ed? > + > + if (need_resched()) > + cond_resched(); > + > + ret = sgx_encl_add_page(encl, addp->addr + i*PAGE_SIZE, > + addp->src + i*PAGE_SIZE, > + &secinfo, addp->mrmask); > + } > return ret; > } > > @@ -823,8 +845,8 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) > case SGX_IOC_ENCLAVE_CREATE: > handler = sgx_ioc_enclave_create; > break; > - case SGX_IOC_ENCLAVE_ADD_PAGE: > - handler = sgx_ioc_enclave_add_page; > + case SGX_IOC_ENCLAVE_ADD_PAGES: > + handler = sgx_ioc_enclave_add_pages; > break; > case SGX_IOC_ENCLAVE_INIT: > handler = sgx_ioc_enclave_init; > -- > 2.21.0
On Sun, Jun 02, 2019 at 11:26:09PM -0700, Xing, Cedric wrote: > > From: Christopherson, Sean J > > Sent: Friday, May 31, 2019 4:32 PM > > > > +/** > > + * sgx_ioc_enclave_add_pages - handler for %SGX_IOC_ENCLAVE_ADD_PAGES > > + * > > + * @filep: open file to /dev/sgx > > + * @cmd: the command value > > + * @arg: pointer to an &sgx_enclave_add_page instance > > + * > > + * Add a range of pages to an uninitialized enclave (EADD), and > > +optionally > > + * extend the enclave's measurement with the contents of the page (EEXTEND). > > + * The range of pages must be virtually contiguous. The SECINFO and > > + * measurement maskare applied to all pages, i.e. pages with different > > + * properties must be added in separate calls. > > + * > > + * EADD and EEXTEND are done asynchronously via worker threads. A > > +successful > > + * sgx_ioc_enclave_add_page() only indicates the pages have been added > > +to the > > + * work queue, it does not guarantee adding the pages to the enclave > > +will > > + * succeed. > > + * > > + * Return: > > + * 0 on success, > > + * -errno otherwise > > + */ > > +static long sgx_ioc_enclave_add_pages(struct file *filep, unsigned int cmd, > > + unsigned long arg) > > +{ > > + struct sgx_enclave_add_pages *addp = (void *)arg; > > + struct sgx_encl *encl = filep->private_data; > > + struct sgx_secinfo secinfo; > > + unsigned int i; > > + int ret; > > + > > + if (copy_from_user(&secinfo, (void __user *)addp->secinfo, > > + sizeof(secinfo))) > > + return -EFAULT; > > + > > + for (i = 0, ret = 0; i < addp->nr_pages && !ret; i++) { > > + if (signal_pending(current)) > > + return -ERESTARTSYS; > > If interrupted, how would user mode code know how many pages have been EADD'ed? Hmm, updating nr_pages would be fairly simple and shouldn't confuse userspace, e.g. as opposed to overloading the return value. > > + > > + if (need_resched()) > > + cond_resched(); > > + > > + ret = sgx_encl_add_page(encl, addp->addr + i*PAGE_SIZE, > > + addp->src + i*PAGE_SIZE, > > + &secinfo, addp->mrmask); > > + } > > return ret; > > } > > > > @@ -823,8 +845,8 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) > > case SGX_IOC_ENCLAVE_CREATE: > > handler = sgx_ioc_enclave_create; > > break; > > - case SGX_IOC_ENCLAVE_ADD_PAGE: > > - handler = sgx_ioc_enclave_add_page; > > + case SGX_IOC_ENCLAVE_ADD_PAGES: > > + handler = sgx_ioc_enclave_add_pages; > > break; > > case SGX_IOC_ENCLAVE_INIT: > > handler = sgx_ioc_enclave_init; > > -- > > 2.21.0 >
On 5/31/19 4:31 PM, Sean Christopherson wrote: > -struct sgx_enclave_add_page { > +struct sgx_enclave_add_pages { > __u64 addr; > __u64 src; > __u64 secinfo; > + __u32 nr_pages; > __u16 mrmask; > } __attribute__((__packed__)); IMNHO this follows a user interface anti-pattern: exposing page sizes where not strictly required. Think of how this would look to an application if page size was variable. With this interface, they always need to scale their operations by page size instead of just aligning it. BTW, why is nr_pages a u32? Do we never envision a case where you can add more than 4TB of memory to an enclave? ;)
On Mon, Jun 03, 2019 at 01:14:45PM -0700, Dave Hansen wrote: > On 5/31/19 4:31 PM, Sean Christopherson wrote: > > -struct sgx_enclave_add_page { > > +struct sgx_enclave_add_pages { > > __u64 addr; > > __u64 src; > > __u64 secinfo; > > + __u32 nr_pages; > > __u16 mrmask; > > } __attribute__((__packed__)); > > IMNHO this follows a user interface anti-pattern: exposing page sizes > where not strictly required. > > Think of how this would look to an application if page size was > variable. With this interface, they always need to scale their > operations by page size instead of just aligning it. I briefly considered taking size in bytes, but I took a shortcut because EPC pages are architecturally defined to be 4k sized and aligned. That being said, I don't necessarily disagree, especially if nr_pages isn't squeezed into a u32. > BTW, why is nr_pages a u32? Do we never envision a case where you can > add more than 4TB of memory to an enclave? ;) Heh, fair enough. IIRC, a while back someone posted about having problems building a 512gb enclave in a 92mb EPC... How about this for the intermediate patch: struct sgx_enclave_add_region { __u64 addr; __u64 src; __u64 size; __u64 secinfo; __u16 mrmask; __u16 reserved16; __u32 reserved; } and with the flags field: struct sgx_enclave_add_region { __u64 addr; __u64 src; __u64 size; __u64 secinfo; __u16 mrmask; __u16 flags; __u32 reserved; }
On Mon, Jun 03, 2019 at 01:08:04PM -0700, Sean Christopherson wrote: > On Sun, Jun 02, 2019 at 11:26:09PM -0700, Xing, Cedric wrote: > > > From: Christopherson, Sean J > > > Sent: Friday, May 31, 2019 4:32 PM > > > > > > +/** > > > + * sgx_ioc_enclave_add_pages - handler for %SGX_IOC_ENCLAVE_ADD_PAGES > > > + * > > > + * @filep: open file to /dev/sgx > > > + * @cmd: the command value > > > + * @arg: pointer to an &sgx_enclave_add_page instance > > > + * > > > + * Add a range of pages to an uninitialized enclave (EADD), and > > > +optionally > > > + * extend the enclave's measurement with the contents of the page (EEXTEND). > > > + * The range of pages must be virtually contiguous. The SECINFO and > > > + * measurement maskare applied to all pages, i.e. pages with different > > > + * properties must be added in separate calls. > > > + * > > > + * EADD and EEXTEND are done asynchronously via worker threads. A > > > +successful > > > + * sgx_ioc_enclave_add_page() only indicates the pages have been added > > > +to the > > > + * work queue, it does not guarantee adding the pages to the enclave > > > +will > > > + * succeed. > > > + * > > > + * Return: > > > + * 0 on success, > > > + * -errno otherwise > > > + */ > > > +static long sgx_ioc_enclave_add_pages(struct file *filep, unsigned int cmd, > > > + unsigned long arg) > > > +{ > > > + struct sgx_enclave_add_pages *addp = (void *)arg; > > > + struct sgx_encl *encl = filep->private_data; > > > + struct sgx_secinfo secinfo; > > > + unsigned int i; > > > + int ret; > > > + > > > + if (copy_from_user(&secinfo, (void __user *)addp->secinfo, > > > + sizeof(secinfo))) > > > + return -EFAULT; > > > + > > > + for (i = 0, ret = 0; i < addp->nr_pages && !ret; i++) { > > > + if (signal_pending(current)) > > > + return -ERESTARTSYS; > > > > If interrupted, how would user mode code know how many pages have been EADD'ed? > > Hmm, updating nr_pages would be fairly simple and shouldn't confuse > userspace, e.g. as opposed to overloading the return value. Or maybe update @addr and @src as well? That would allow userspace to re-invoke the ioctl() without having to modify the struct.
On 6/3/19 1:37 PM, Sean Christopherson wrote: > Heh, fair enough. IIRC, a while back someone posted about having problems > building a 512gb enclave in a 92mb EPC... > > How about this for the intermediate patch: > > struct sgx_enclave_add_region { > __u64 addr; > __u64 src; > __u64 size; > __u64 secinfo; > __u16 mrmask; > __u16 reserved16; > __u32 reserved; > } Looks fine to me.
> From: Christopherson, Sean J > Sent: Monday, June 03, 2019 1:40 PM > > On Mon, Jun 03, 2019 at 01:08:04PM -0700, Sean Christopherson wrote: > > On Sun, Jun 02, 2019 at 11:26:09PM -0700, Xing, Cedric wrote: > > > > From: Christopherson, Sean J > > > > Sent: Friday, May 31, 2019 4:32 PM > > > > > > > > +/** > > > > + * sgx_ioc_enclave_add_pages - handler for > > > > +%SGX_IOC_ENCLAVE_ADD_PAGES > > > > + * > > > > + * @filep: open file to /dev/sgx > > > > + * @cmd: the command value > > > > + * @arg: pointer to an &sgx_enclave_add_page instance > > > > + * > > > > + * Add a range of pages to an uninitialized enclave (EADD), and > > > > +optionally > > > > + * extend the enclave's measurement with the contents of the page > (EEXTEND). > > > > + * The range of pages must be virtually contiguous. The SECINFO > > > > +and > > > > + * measurement maskare applied to all pages, i.e. pages with > > > > +different > > > > + * properties must be added in separate calls. > > > > + * > > > > + * EADD and EEXTEND are done asynchronously via worker threads. > > > > +A successful > > > > + * sgx_ioc_enclave_add_page() only indicates the pages have been > > > > +added to the > > > > + * work queue, it does not guarantee adding the pages to the > > > > +enclave will > > > > + * succeed. > > > > + * > > > > + * Return: > > > > + * 0 on success, > > > > + * -errno otherwise > > > > + */ > > > > +static long sgx_ioc_enclave_add_pages(struct file *filep, > unsigned int cmd, > > > > + unsigned long arg) > > > > +{ > > > > + struct sgx_enclave_add_pages *addp = (void *)arg; > > > > + struct sgx_encl *encl = filep->private_data; > > > > + struct sgx_secinfo secinfo; > > > > + unsigned int i; > > > > + int ret; > > > > + > > > > + if (copy_from_user(&secinfo, (void __user *)addp->secinfo, > > > > + sizeof(secinfo))) > > > > + return -EFAULT; > > > > + > > > > + for (i = 0, ret = 0; i < addp->nr_pages && !ret; i++) { > > > > + if (signal_pending(current)) > > > > + return -ERESTARTSYS; > > > > > > If interrupted, how would user mode code know how many pages have > been EADD'ed? > > > > Hmm, updating nr_pages would be fairly simple and shouldn't confuse > > userspace, e.g. as opposed to overloading the return value. > > Or maybe update @addr and @src as well? That would allow userspace to > re-invoke the ioctl() without having to modify the struct. How about returning the number of pages (or bytes) EADD'ed, similar to write() syscall?
> -----Original Message----- > From: Christopherson, Sean J > Sent: Monday, June 03, 2019 1:37 PM > To: Hansen, Dave <dave.hansen@intel.com> > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; Andy Lutomirski > <luto@kernel.org>; Xing, Cedric <cedric.xing@intel.com>; Stephen Smalley > <sds@tycho.nsa.gov>; James Morris <jmorris@namei.org>; Serge E . Hallyn > <serge@hallyn.com>; LSM List <linux-security-module@vger.kernel.org>; > Paul Moore <paul@paul-moore.com>; Eric Paris <eparis@parisplace.org>; > selinux@vger.kernel.org; Jethro Beekman <jethro@fortanix.com>; Thomas > Gleixner <tglx@linutronix.de>; Linus Torvalds <torvalds@linux- > foundation.org>; LKML <linux-kernel@vger.kernel.org>; X86 ML > <x86@kernel.org>; linux-sgx@vger.kernel.org; Andrew Morton <akpm@linux- > foundation.org>; nhorman@redhat.com; npmccallum@redhat.com; Ayoun, Serge > <serge.ayoun@intel.com>; Katz-zamir, Shay <shay.katz-zamir@intel.com>; > Huang, Haitao <haitao.huang@intel.com>; Andy Shevchenko > <andriy.shevchenko@linux.intel.com>; Svahn, Kai <kai.svahn@intel.com>; > Borislav Petkov <bp@alien8.de>; Josh Triplett <josh@joshtriplett.org>; > Huang, Kai <kai.huang@intel.com>; David Rientjes <rientjes@google.com>; > Roberts, William C <william.c.roberts@intel.com>; Tricca, Philip B > <philip.b.tricca@intel.com> > Subject: Re: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple > pages in single ioctl() > > On Mon, Jun 03, 2019 at 01:14:45PM -0700, Dave Hansen wrote: > > On 5/31/19 4:31 PM, Sean Christopherson wrote: > > > -struct sgx_enclave_add_page { > > > +struct sgx_enclave_add_pages { > > > __u64 addr; > > > __u64 src; > > > __u64 secinfo; > > > + __u32 nr_pages; > > > __u16 mrmask; > > > } __attribute__((__packed__)); > > > > IMNHO this follows a user interface anti-pattern: exposing page sizes > > where not strictly required. > > > > Think of how this would look to an application if page size was > > variable. With this interface, they always need to scale their > > operations by page size instead of just aligning it. > > I briefly considered taking size in bytes, but I took a shortcut because > EPC pages are architecturally defined to be 4k sized and aligned. That > being said, I don't necessarily disagree, especially if nr_pages isn't > squeezed into a u32. > > > BTW, why is nr_pages a u32? Do we never envision a case where you can > > add more than 4TB of memory to an enclave? ;) > > Heh, fair enough. IIRC, a while back someone posted about having > problems building a 512gb enclave in a 92mb EPC... > > How about this for the intermediate patch: > > struct sgx_enclave_add_region { > __u64 addr; > __u64 src; > __u64 size; > __u64 secinfo; > __u16 mrmask; > __u16 reserved16; > __u32 reserved; > } > > and with the flags field: > > struct sgx_enclave_add_region { > __u64 addr; > __u64 src; > __u64 size; > __u64 secinfo; > __u16 mrmask; > __u16 flags; What is "flags" here? > __u32 reserved; > }
On Mon, Jun 03, 2019 at 04:45:45PM -0700, Xing, Cedric wrote: > > From: Christopherson, Sean J > > Sent: Monday, June 03, 2019 1:40 PM > > > > On Mon, Jun 03, 2019 at 01:08:04PM -0700, Sean Christopherson wrote: > > > On Sun, Jun 02, 2019 at 11:26:09PM -0700, Xing, Cedric wrote: > > > > > From: Christopherson, Sean J > > > > > Sent: Friday, May 31, 2019 4:32 PM > > > > > > > > > > +/** > > > > > + * sgx_ioc_enclave_add_pages - handler for > > > > > +%SGX_IOC_ENCLAVE_ADD_PAGES > > > > > + * > > > > > + * @filep: open file to /dev/sgx > > > > > + * @cmd: the command value > > > > > + * @arg: pointer to an &sgx_enclave_add_page instance > > > > > + * > > > > > + * Add a range of pages to an uninitialized enclave (EADD), and > > > > > +optionally > > > > > + * extend the enclave's measurement with the contents of the page > > (EEXTEND). > > > > > + * The range of pages must be virtually contiguous. The SECINFO > > > > > +and > > > > > + * measurement maskare applied to all pages, i.e. pages with > > > > > +different > > > > > + * properties must be added in separate calls. > > > > > + * > > > > > + * EADD and EEXTEND are done asynchronously via worker threads. > > > > > +A successful > > > > > + * sgx_ioc_enclave_add_page() only indicates the pages have been > > > > > +added to the > > > > > + * work queue, it does not guarantee adding the pages to the > > > > > +enclave will > > > > > + * succeed. > > > > > + * > > > > > + * Return: > > > > > + * 0 on success, > > > > > + * -errno otherwise > > > > > + */ > > > > > +static long sgx_ioc_enclave_add_pages(struct file *filep, > > unsigned int cmd, > > > > > + unsigned long arg) > > > > > +{ > > > > > + struct sgx_enclave_add_pages *addp = (void *)arg; > > > > > + struct sgx_encl *encl = filep->private_data; > > > > > + struct sgx_secinfo secinfo; > > > > > + unsigned int i; > > > > > + int ret; > > > > > + > > > > > + if (copy_from_user(&secinfo, (void __user *)addp->secinfo, > > > > > + sizeof(secinfo))) > > > > > + return -EFAULT; > > > > > + > > > > > + for (i = 0, ret = 0; i < addp->nr_pages && !ret; i++) { > > > > > + if (signal_pending(current)) > > > > > + return -ERESTARTSYS; > > > > > > > > If interrupted, how would user mode code know how many pages have > > been EADD'ed? > > > > > > Hmm, updating nr_pages would be fairly simple and shouldn't confuse > > > userspace, e.g. as opposed to overloading the return value. > > > > Or maybe update @addr and @src as well? That would allow userspace to > > re-invoke the ioctl() without having to modify the struct. > > How about returning the number of pages (or bytes) EADD'ed, similar to > write() syscall? I thought about that as well, but I think it'd be useful to update the offset on any failure, not just -ERESTARTSYS.
On Mon, Jun 03, 2019 at 04:48:47PM -0700, Xing, Cedric wrote: > > How about this for the intermediate patch: > > > > struct sgx_enclave_add_region { > > __u64 addr; > > __u64 src; > > __u64 size; > > __u64 secinfo; > > __u16 mrmask; > > __u16 reserved16; > > __u32 reserved; > > } > > > > and with the flags field: > > > > struct sgx_enclave_add_region { > > __u64 addr; > > __u64 src; > > __u64 size; > > __u64 secinfo; > > __u16 mrmask; > > __u16 flags; > > What is "flags" here? In the RFC, @flags holds SGX_ALLOW_{READ,WRITE,EXEC}. > > > __u32 reserved; > > }
On Fri, May 31, 2019 at 04:31:53PM -0700, Sean Christopherson wrote: > ...to improve performance when building enclaves by reducing the number > of user<->system transitions. Rather than provide arbitrary batching, > e.g. with per-page SECINFO and mrmask, take advantage of the fact that > any sane enclave will have large swaths of pages with identical > properties, e.g. code vs. data sections. > > For simplicity and stability in the initial implementation, loop over > the existing add page flow instead of taking a more agressive approach, > which would require tracking transitions between VMAs and holding > mmap_sem for an extended duration. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> I think this completely ruins the rest of the series. We should first get the model for security done (including documentation). I would even send v21 with just that update because this series does not even apply to the mainline. I would request an update to the series with just the change to the security model. Also the very first should be dropped as it is completely unrelated cosmetic fix. /Jarkko
On Mon, Jun 3, 2019 at 1:39 PM Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Mon, Jun 03, 2019 at 01:08:04PM -0700, Sean Christopherson wrote: > > On Sun, Jun 02, 2019 at 11:26:09PM -0700, Xing, Cedric wrote: > > > > From: Christopherson, Sean J > > > > Sent: Friday, May 31, 2019 4:32 PM > > > > > > > > +/** > > > > + * sgx_ioc_enclave_add_pages - handler for %SGX_IOC_ENCLAVE_ADD_PAGES > > > > + * > > > > + * @filep: open file to /dev/sgx > > > > + * @cmd: the command value > > > > + * @arg: pointer to an &sgx_enclave_add_page instance > > > > + * > > > > + * Add a range of pages to an uninitialized enclave (EADD), and > > > > +optionally > > > > + * extend the enclave's measurement with the contents of the page (EEXTEND). > > > > + * The range of pages must be virtually contiguous. The SECINFO and > > > > + * measurement maskare applied to all pages, i.e. pages with different > > > > + * properties must be added in separate calls. > > > > + * > > > > + * EADD and EEXTEND are done asynchronously via worker threads. A > > > > +successful > > > > + * sgx_ioc_enclave_add_page() only indicates the pages have been added > > > > +to the > > > > + * work queue, it does not guarantee adding the pages to the enclave > > > > +will > > > > + * succeed. > > > > + * > > > > + * Return: > > > > + * 0 on success, > > > > + * -errno otherwise > > > > + */ > > > > +static long sgx_ioc_enclave_add_pages(struct file *filep, unsigned int cmd, > > > > + unsigned long arg) > > > > +{ > > > > + struct sgx_enclave_add_pages *addp = (void *)arg; > > > > + struct sgx_encl *encl = filep->private_data; > > > > + struct sgx_secinfo secinfo; > > > > + unsigned int i; > > > > + int ret; > > > > + > > > > + if (copy_from_user(&secinfo, (void __user *)addp->secinfo, > > > > + sizeof(secinfo))) > > > > + return -EFAULT; > > > > + > > > > + for (i = 0, ret = 0; i < addp->nr_pages && !ret; i++) { > > > > + if (signal_pending(current)) > > > > + return -ERESTARTSYS; > > > > > > If interrupted, how would user mode code know how many pages have been EADD'ed? > > > > Hmm, updating nr_pages would be fairly simple and shouldn't confuse > > userspace, e.g. as opposed to overloading the return value. > > Or maybe update @addr and @src as well? That would allow userspace to > re-invoke the ioctl() without having to modify the struct. If you're going to use -ERESTARTSYS, that's the way to go. -EINTR would be an alternative. A benefit of -ERESTARTSYS is that, with -EINTR, it wouldn't be that surprising for user code to simply fail to handle it.
> From: Andy Lutomirski [mailto:luto@kernel.org] > Sent: Tuesday, June 04, 2019 1:18 PM > > On Mon, Jun 3, 2019 at 1:39 PM Sean Christopherson > <sean.j.christopherson@intel.com> wrote: > > > > On Mon, Jun 03, 2019 at 01:08:04PM -0700, Sean Christopherson wrote: > > > On Sun, Jun 02, 2019 at 11:26:09PM -0700, Xing, Cedric wrote: > > > > > From: Christopherson, Sean J > > > > > Sent: Friday, May 31, 2019 4:32 PM > > > > > > > > > > +/** > > > > > + * sgx_ioc_enclave_add_pages - handler for > > > > > +%SGX_IOC_ENCLAVE_ADD_PAGES > > > > > + * > > > > > + * @filep: open file to /dev/sgx > > > > > + * @cmd: the command value > > > > > + * @arg: pointer to an &sgx_enclave_add_page instance > > > > > + * > > > > > + * Add a range of pages to an uninitialized enclave (EADD), and > > > > > +optionally > > > > > + * extend the enclave's measurement with the contents of the > page (EEXTEND). > > > > > + * The range of pages must be virtually contiguous. The > > > > > +SECINFO and > > > > > + * measurement maskare applied to all pages, i.e. pages with > > > > > +different > > > > > + * properties must be added in separate calls. > > > > > + * > > > > > + * EADD and EEXTEND are done asynchronously via worker threads. > > > > > +A successful > > > > > + * sgx_ioc_enclave_add_page() only indicates the pages have > > > > > +been added to the > > > > > + * work queue, it does not guarantee adding the pages to the > > > > > +enclave will > > > > > + * succeed. > > > > > + * > > > > > + * Return: > > > > > + * 0 on success, > > > > > + * -errno otherwise > > > > > + */ > > > > > +static long sgx_ioc_enclave_add_pages(struct file *filep, > unsigned int cmd, > > > > > + unsigned long arg) { struct > > > > > +sgx_enclave_add_pages *addp = (void *)arg; struct sgx_encl > > > > > +*encl = filep->private_data; struct sgx_secinfo secinfo; > > > > > +unsigned int i; int ret; > > > > > + > > > > > + if (copy_from_user(&secinfo, (void __user *)addp->secinfo, > > > > > + sizeof(secinfo))) > > > > > + return -EFAULT; > > > > > + > > > > > + for (i = 0, ret = 0; i < addp->nr_pages && !ret; i++) { > > > > > + if (signal_pending(current)) > > > > > + return -ERESTARTSYS; > > > > > > > > If interrupted, how would user mode code know how many pages have > been EADD'ed? > > > > > > Hmm, updating nr_pages would be fairly simple and shouldn't confuse > > > userspace, e.g. as opposed to overloading the return value. > > > > Or maybe update @addr and @src as well? That would allow userspace to > > re-invoke the ioctl() without having to modify the struct. > > If you're going to use -ERESTARTSYS, that's the way to go. -EINTR would > be an alternative. A benefit of -ERESTARTSYS is that, with -EINTR, it > wouldn't be that surprising for user code to simply fail to handle it. -EINTR means the call was interrupted before anything could be done. Am I correct? But in this case some pages have been processed already so I guess we cannot return any error code. I think it more reasonable to return the number of pages (or bytes) processed.
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h index 9ed690a38c70..4a12d6abbcb7 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 \ + _IOW(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 \ @@ -32,21 +32,22 @@ struct sgx_enclave_create { }; /** - * struct sgx_enclave_add_page - parameter structure for the - * %SGX_IOC_ENCLAVE_ADD_PAGE ioctl - * @addr: address within the ELRANGE - * @src: address for the page data - * @secinfo: address for the SECINFO data - * @mrmask: bitmask for the measured 256 byte chunks + * struct sgx_enclave_add_pages - parameter structure for the + * %SGX_IOC_ENCLAVE_ADD_PAGES ioctl + * @addr: start address within the ELRANGE + * @src: start address for the pages' data + * @secinfo: address for the SECINFO data (common to all pages) + * @nr_pages: number of pages (must be virtually contiguous) + * @mrmask: bitmask for the measured 256 byte chunks (common to all pages) */ -struct sgx_enclave_add_page { +struct sgx_enclave_add_pages { __u64 addr; __u64 src; __u64 secinfo; + __u32 nr_pages; __u16 mrmask; } __attribute__((__packed__)); - /** * struct sgx_enclave_init - parameter structure for the * %SGX_IOC_ENCLAVE_INIT ioctl diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c index a27ec26a9350..6acfcbdeca9a 100644 --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c @@ -487,10 +487,9 @@ static int sgx_validate_tcs(struct sgx_encl *encl, struct sgx_tcs *tcs) return 0; } -static int __sgx_encl_add_page(struct sgx_encl *encl, +static int sgx_encl_queue_page(struct sgx_encl *encl, struct sgx_encl_page *encl_page, - void *data, - struct sgx_secinfo *secinfo, + void *data, struct sgx_secinfo *secinfo, unsigned int mrmask) { unsigned long page_index = sgx_encl_get_index(encl, encl_page); @@ -529,9 +528,9 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, return 0; } -static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr, - void *data, struct sgx_secinfo *secinfo, - unsigned int mrmask) +static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr, + void *data, struct sgx_secinfo *secinfo, + unsigned int mrmask) { u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK; struct sgx_encl_page *encl_page; @@ -563,7 +562,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr, goto out; } - ret = __sgx_encl_add_page(encl, encl_page, data, secinfo, mrmask); + ret = sgx_encl_queue_page(encl, encl_page, data, secinfo, mrmask); if (ret) { radix_tree_delete(&encl_page->encl->page_tree, PFN_DOWN(encl_page->desc)); @@ -575,56 +574,79 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr, return ret; } -/** - * sgx_ioc_enclave_add_page - handler for %SGX_IOC_ENCLAVE_ADD_PAGE - * - * @filep: open file to /dev/sgx - * @cmd: the command value - * @arg: pointer to an &sgx_enclave_add_page instance - * - * Add a page to an uninitialized enclave (EADD), and optionally extend the - * enclave's measurement with the contents of the page (EEXTEND). EADD and - * EEXTEND are done asynchronously via worker threads. A successful - * sgx_ioc_enclave_add_page() only indicates the page has been added to the - * work queue, it does not guarantee adding the page to the enclave will - * succeed. - * - * Return: - * 0 on success, - * -errno otherwise - */ -static long sgx_ioc_enclave_add_page(struct file *filep, unsigned int cmd, - unsigned long arg) +static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr, + unsigned long src, struct sgx_secinfo *secinfo, + unsigned int mrmask) { - struct sgx_enclave_add_page *addp = (void *)arg; - struct sgx_encl *encl = filep->private_data; - struct sgx_secinfo secinfo; struct page *data_page; void *data; int ret; - if (copy_from_user(&secinfo, (void __user *)addp->secinfo, - sizeof(secinfo))) - return -EFAULT; - data_page = alloc_page(GFP_HIGHUSER); if (!data_page) return -ENOMEM; data = kmap(data_page); - if (copy_from_user((void *)data, (void __user *)addp->src, PAGE_SIZE)) { + if (copy_from_user((void *)data, (void __user *)src, PAGE_SIZE)) { ret = -EFAULT; goto out; } - ret = sgx_encl_add_page(encl, addp->addr, data, &secinfo, addp->mrmask); - if (ret) - goto out; - + ret = __sgx_encl_add_page(encl, addr, data, secinfo, mrmask); out: kunmap(data_page); __free_page(data_page); + + return ret; +} + +/** + * sgx_ioc_enclave_add_pages - handler for %SGX_IOC_ENCLAVE_ADD_PAGES + * + * @filep: open file to /dev/sgx + * @cmd: the command value + * @arg: pointer to an &sgx_enclave_add_page instance + * + * Add a range of pages to an uninitialized enclave (EADD), and optionally + * extend the enclave's measurement with the contents of the page (EEXTEND). + * The range of pages must be virtually contiguous. The SECINFO and + * measurement maskare applied to all pages, i.e. pages with different + * properties must be added in separate calls. + * + * EADD and EEXTEND are done asynchronously via worker threads. A successful + * sgx_ioc_enclave_add_page() only indicates the pages have been added to the + * work queue, it does not guarantee adding the pages to the enclave will + * succeed. + * + * Return: + * 0 on success, + * -errno otherwise + */ +static long sgx_ioc_enclave_add_pages(struct file *filep, unsigned int cmd, + unsigned long arg) +{ + struct sgx_enclave_add_pages *addp = (void *)arg; + struct sgx_encl *encl = filep->private_data; + struct sgx_secinfo secinfo; + unsigned int i; + int ret; + + if (copy_from_user(&secinfo, (void __user *)addp->secinfo, + sizeof(secinfo))) + return -EFAULT; + + for (i = 0, ret = 0; i < addp->nr_pages && !ret; i++) { + if (signal_pending(current)) + return -ERESTARTSYS; + + if (need_resched()) + cond_resched(); + + ret = sgx_encl_add_page(encl, addp->addr + i*PAGE_SIZE, + addp->src + i*PAGE_SIZE, + &secinfo, addp->mrmask); + } return ret; } @@ -823,8 +845,8 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) case SGX_IOC_ENCLAVE_CREATE: handler = sgx_ioc_enclave_create; break; - case SGX_IOC_ENCLAVE_ADD_PAGE: - handler = sgx_ioc_enclave_add_page; + case SGX_IOC_ENCLAVE_ADD_PAGES: + handler = sgx_ioc_enclave_add_pages; break; case SGX_IOC_ENCLAVE_INIT: handler = sgx_ioc_enclave_init;
...to improve performance when building enclaves by reducing the number of user<->system transitions. Rather than provide arbitrary batching, e.g. with per-page SECINFO and mrmask, take advantage of the fact that any sane enclave will have large swaths of pages with identical properties, e.g. code vs. data sections. For simplicity and stability in the initial implementation, loop over the existing add page flow instead of taking a more agressive approach, which would require tracking transitions between VMAs and holding mmap_sem for an extended duration. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/include/uapi/asm/sgx.h | 21 ++--- arch/x86/kernel/cpu/sgx/driver/ioctl.c | 104 +++++++++++++++---------- 2 files changed, 74 insertions(+), 51 deletions(-)