[RFC,3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()
diff mbox series

Message ID 20190531233159.30992-4-sean.j.christopherson@intel.com
State New
Headers show
Series
  • security: x86/sgx: SGX vs. LSM
Related show

Commit Message

Sean Christopherson May 31, 2019, 11:31 p.m. UTC
...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(-)

Comments

Xing, Cedric June 3, 2019, 6:26 a.m. UTC | #1
> 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
Sean Christopherson June 3, 2019, 8:08 p.m. UTC | #2
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
>
Dave Hansen June 3, 2019, 8:14 p.m. UTC | #3
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? ;)
Sean Christopherson June 3, 2019, 8:37 p.m. UTC | #4
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;
	}
Sean Christopherson June 3, 2019, 8:39 p.m. UTC | #5
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.
Dave Hansen June 3, 2019, 8:39 p.m. UTC | #6
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.
Xing, Cedric June 3, 2019, 11:45 p.m. UTC | #7
> 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?
Xing, Cedric June 3, 2019, 11:48 p.m. UTC | #8
> -----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;
> 	}
Sean Christopherson June 4, 2019, 12:54 a.m. UTC | #9
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.
Sean Christopherson June 4, 2019, 12:55 a.m. UTC | #10
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;
> > 	}
Jarkko Sakkinen June 4, 2019, 11:55 a.m. UTC | #11
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
Andy Lutomirski June 4, 2019, 8:18 p.m. UTC | #12
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.
Xing, Cedric June 4, 2019, 10:02 p.m. UTC | #13
> 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.

Patch
diff mbox series

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;