diff mbox

[intel-sgx-kernel-dev] intel_sgx: use VM_DONTCOPY

Message ID 20170112143316.15094-1-jarkko.sakkinen@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jarkko Sakkinen Jan. 12, 2017, 2:33 p.m. UTC 
In order to get stable and well defined semantics use VM_DONTCOPY
instead of custom hacks that eventually break in a way or another.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/platform/x86/intel_sgx_main.c |  7 ++-----
 drivers/platform/x86/intel_sgx_vma.c  | 27 +--------------------------
 2 files changed, 3 insertions(+), 31 deletions(-)

Comments

Sean Christopherson Jan. 12, 2017, 3:08 p.m. UTC | #1 
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> In order to get stable and well defined semantics use VM_DONTCOPY
> instead of custom hacks that eventually break in a way or another.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  drivers/platform/x86/intel_sgx_main.c |  7 ++-----
>  drivers/platform/x86/intel_sgx_vma.c  | 27 +--------------------------
>  2 files changed, 3 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_sgx_main.c
> b/drivers/platform/x86/intel_sgx_main.c index 93dd708..b5e414a 100644
> --- a/drivers/platform/x86/intel_sgx_main.c
> +++ b/drivers/platform/x86/intel_sgx_main.c
> @@ -103,11 +103,8 @@ long sgx_compat_ioctl(struct file *filep, unsigned int
> cmd, unsigned long arg) static int sgx_mmap(struct file *file, struct
> vm_area_struct *vma) {
>  	vma->vm_ops = &sgx_vm_ops;
> -#if !defined(VM_RESERVED)
> -	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO;
> -#else
> -	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_RESERVED | VM_IO;
> -#endif
> +	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO |
> +			 VM_DONTCOPY;
>  
>  	return 0;
>  }
> diff --git a/drivers/platform/x86/intel_sgx_vma.c
> b/drivers/platform/x86/intel_sgx_vma.c index e670405..cba50c2 100644
> --- a/drivers/platform/x86/intel_sgx_vma.c
> +++ b/drivers/platform/x86/intel_sgx_vma.c
> @@ -71,42 +71,17 @@
>  static void sgx_vma_open(struct vm_area_struct *vma)
>  {
>  	struct sgx_encl *encl = vma->vm_private_data;
> -
> -	/* When forking for the second time vm_private_data is already set to
> -	 * NULL.
> -	 */
> -	if (!encl) {
> -		zap_vma_ptes(vma, vma->vm_start, vma->vm_end -
> vma->vm_start);
> -		return;
> -	}
> -
> -	/* Invalidate enclave when the process has been forked. */
> -	mutex_lock(&encl->lock);
> -	if (encl->mm != vma->vm_mm) {
> -		sgx_invalidate(encl);
> -		vma->vm_private_data = NULL;
> -	}
> -	mutex_unlock(&encl->lock);
> -
> -	if (vma->vm_private_data)
> -		kref_get(&encl->refcount);
> +	kref_get(&encl->refcount);
>  }
>  
>  static void sgx_vma_close(struct vm_area_struct *vma)
>  {
>  	struct sgx_encl *encl = vma->vm_private_data;
>  
> -	/* When forking for the second time vm_private_data is already set to
> -	 * NULL.
> -	 */
> -	if (!encl)
> -		return;
> -
>  	mutex_lock(&encl->lock);
>  	zap_vma_ptes(vma, vma->vm_start, vma->vm_end - vma->vm_start);
>  	encl->flags |= SGX_ENCL_DEAD;
>  	mutex_unlock(&encl->lock);
> -
>  	kref_put(&encl->refcount, sgx_encl_release);
>  }

The !encl checks need to be retained in both sgx_vma_close and sgx_vma_open.  vm_private_data is not set until user space calls SGX_IOC_ENCLAVE_CREATE and it is legal (and necessary) for user space to make calls to mmap/munmap prior to SGX_IOC_ENCLAVE_CREATE.
Jarkko Sakkinen Jan. 12, 2017, 4:54 p.m. UTC | #2 
On Thu, Jan 12, 2017 at 03:08:41PM +0000, Christopherson, Sean J wrote:
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > In order to get stable and well defined semantics use VM_DONTCOPY
> > instead of custom hacks that eventually break in a way or another.
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  drivers/platform/x86/intel_sgx_main.c |  7 ++-----
> >  drivers/platform/x86/intel_sgx_vma.c  | 27 +--------------------------
> >  2 files changed, 3 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel_sgx_main.c
> > b/drivers/platform/x86/intel_sgx_main.c index 93dd708..b5e414a 100644
> > --- a/drivers/platform/x86/intel_sgx_main.c
> > +++ b/drivers/platform/x86/intel_sgx_main.c
> > @@ -103,11 +103,8 @@ long sgx_compat_ioctl(struct file *filep, unsigned int
> > cmd, unsigned long arg) static int sgx_mmap(struct file *file, struct
> > vm_area_struct *vma) {
> >  	vma->vm_ops = &sgx_vm_ops;
> > -#if !defined(VM_RESERVED)
> > -	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO;
> > -#else
> > -	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_RESERVED | VM_IO;
> > -#endif
> > +	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO |
> > +			 VM_DONTCOPY;
> >  
> >  	return 0;
> >  }
> > diff --git a/drivers/platform/x86/intel_sgx_vma.c
> > b/drivers/platform/x86/intel_sgx_vma.c index e670405..cba50c2 100644
> > --- a/drivers/platform/x86/intel_sgx_vma.c
> > +++ b/drivers/platform/x86/intel_sgx_vma.c
> > @@ -71,42 +71,17 @@
> >  static void sgx_vma_open(struct vm_area_struct *vma)
> >  {
> >  	struct sgx_encl *encl = vma->vm_private_data;
> > -
> > -	/* When forking for the second time vm_private_data is already set to
> > -	 * NULL.
> > -	 */
> > -	if (!encl) {
> > -		zap_vma_ptes(vma, vma->vm_start, vma->vm_end -
> > vma->vm_start);
> > -		return;
> > -	}
> > -
> > -	/* Invalidate enclave when the process has been forked. */
> > -	mutex_lock(&encl->lock);
> > -	if (encl->mm != vma->vm_mm) {
> > -		sgx_invalidate(encl);
> > -		vma->vm_private_data = NULL;
> > -	}
> > -	mutex_unlock(&encl->lock);
> > -
> > -	if (vma->vm_private_data)
> > -		kref_get(&encl->refcount);
> > +	kref_get(&encl->refcount);
> >  }
> >  
> >  static void sgx_vma_close(struct vm_area_struct *vma)
> >  {
> >  	struct sgx_encl *encl = vma->vm_private_data;
> >  
> > -	/* When forking for the second time vm_private_data is already set to
> > -	 * NULL.
> > -	 */
> > -	if (!encl)
> > -		return;
> > -
> >  	mutex_lock(&encl->lock);
> >  	zap_vma_ptes(vma, vma->vm_start, vma->vm_end - vma->vm_start);
> >  	encl->flags |= SGX_ENCL_DEAD;
> >  	mutex_unlock(&encl->lock);
> > -
> >  	kref_put(&encl->refcount, sgx_encl_release);
> >  }
> 
> The !encl checks need to be retained in both sgx_vma_close and
> sgx_vma_open.  vm_private_data is not set until user space calls
> SGX_IOC_ENCLAVE_CREATE and it is legal (and necessary) for user space
> to make calls to mmap/munmap prior to SGX_IOC_ENCLAVE_CREATE.

Whoops, I forgot to add RFC tag. I only checked that this compiles.
Thanks for the feedback.

/Jarkko
Jarkko Sakkinen Jan. 12, 2017, 6:44 p.m. UTC | #3 
On Thu, Jan 12, 2017 at 06:54:17PM +0200, Jarkko Sakkinen wrote:
> On Thu, Jan 12, 2017 at 03:08:41PM +0000, Christopherson, Sean J wrote:
> > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > > In order to get stable and well defined semantics use VM_DONTCOPY
> > > instead of custom hacks that eventually break in a way or another.
> > > 
> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > ---
> > >  drivers/platform/x86/intel_sgx_main.c |  7 ++-----
> > >  drivers/platform/x86/intel_sgx_vma.c  | 27 +--------------------------
> > >  2 files changed, 3 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/intel_sgx_main.c
> > > b/drivers/platform/x86/intel_sgx_main.c index 93dd708..b5e414a 100644
> > > --- a/drivers/platform/x86/intel_sgx_main.c
> > > +++ b/drivers/platform/x86/intel_sgx_main.c
> > > @@ -103,11 +103,8 @@ long sgx_compat_ioctl(struct file *filep, unsigned int
> > > cmd, unsigned long arg) static int sgx_mmap(struct file *file, struct
> > > vm_area_struct *vma) {
> > >  	vma->vm_ops = &sgx_vm_ops;
> > > -#if !defined(VM_RESERVED)
> > > -	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO;
> > > -#else
> > > -	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_RESERVED | VM_IO;
> > > -#endif
> > > +	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO |
> > > +			 VM_DONTCOPY;
> > >  
> > >  	return 0;
> > >  }
> > > diff --git a/drivers/platform/x86/intel_sgx_vma.c
> > > b/drivers/platform/x86/intel_sgx_vma.c index e670405..cba50c2 100644
> > > --- a/drivers/platform/x86/intel_sgx_vma.c
> > > +++ b/drivers/platform/x86/intel_sgx_vma.c
> > > @@ -71,42 +71,17 @@
> > >  static void sgx_vma_open(struct vm_area_struct *vma)
> > >  {
> > >  	struct sgx_encl *encl = vma->vm_private_data;
> > > -
> > > -	/* When forking for the second time vm_private_data is already set to
> > > -	 * NULL.
> > > -	 */
> > > -	if (!encl) {
> > > -		zap_vma_ptes(vma, vma->vm_start, vma->vm_end -
> > > vma->vm_start);
> > > -		return;
> > > -	}
> > > -
> > > -	/* Invalidate enclave when the process has been forked. */
> > > -	mutex_lock(&encl->lock);
> > > -	if (encl->mm != vma->vm_mm) {
> > > -		sgx_invalidate(encl);
> > > -		vma->vm_private_data = NULL;
> > > -	}
> > > -	mutex_unlock(&encl->lock);
> > > -
> > > -	if (vma->vm_private_data)
> > > -		kref_get(&encl->refcount);
> > > +	kref_get(&encl->refcount);
> > >  }
> > >  
> > >  static void sgx_vma_close(struct vm_area_struct *vma)
> > >  {
> > >  	struct sgx_encl *encl = vma->vm_private_data;
> > >  
> > > -	/* When forking for the second time vm_private_data is already set to
> > > -	 * NULL.
> > > -	 */
> > > -	if (!encl)
> > > -		return;
> > > -
> > >  	mutex_lock(&encl->lock);
> > >  	zap_vma_ptes(vma, vma->vm_start, vma->vm_end - vma->vm_start);
> > >  	encl->flags |= SGX_ENCL_DEAD;
> > >  	mutex_unlock(&encl->lock);
> > > -
> > >  	kref_put(&encl->refcount, sgx_encl_release);
> > >  }
> > 
> > The !encl checks need to be retained in both sgx_vma_close and
> > sgx_vma_open.  vm_private_data is not set until user space calls
> > SGX_IOC_ENCLAVE_CREATE and it is legal (and necessary) for user space
> > to make calls to mmap/munmap prior to SGX_IOC_ENCLAVE_CREATE.
> 
> Whoops, I forgot to add RFC tag. I only checked that this compiles.
> Thanks for the feedback.

Speaking of this issue isn't there a bug that if you do a bunch of
mprotects after mmap but before ECREATE kref_puts will underflow?
Unrelated to this patch.

Just quickly skimmed but I might be ignoring something..

/Jarkko
Sean Christopherson Jan. 12, 2017, 8:41 p.m. UTC | #4 
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > > The !encl checks need to be retained in both sgx_vma_close and
> > > sgx_vma_open.  vm_private_data is not set until user space calls
> > > SGX_IOC_ENCLAVE_CREATE and it is legal (and necessary) for user space
> > > to make calls to mmap/munmap prior to SGX_IOC_ENCLAVE_CREATE.
> > 
> > Whoops, I forgot to add RFC tag. I only checked that this compiles.
> > Thanks for the feedback.
> 
> Speaking of this issue isn't there a bug that if you do a bunch of
> mprotects after mmap but before ECREATE kref_puts will underflow?
> Unrelated to this patch.
> 
> Just quickly skimmed but I might be ignoring something..

kref_puts won't be called because vm_private_data will be null in the VMAs created by mprotect.  The ECREATE call would also fail as sgx_ioc_enclave_create checks to ensure the VMA it finds exactly matches the size of the enclave.
Jarkko Sakkinen Jan. 13, 2017, 4:47 p.m. UTC | #5 
On Thu, Jan 12, 2017 at 08:41:23PM +0000, Christopherson, Sean J wrote:
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > > > The !encl checks need to be retained in both sgx_vma_close and
> > > > sgx_vma_open.  vm_private_data is not set until user space calls
> > > > SGX_IOC_ENCLAVE_CREATE and it is legal (and necessary) for user space
> > > > to make calls to mmap/munmap prior to SGX_IOC_ENCLAVE_CREATE.
> > > 
> > > Whoops, I forgot to add RFC tag. I only checked that this compiles.
> > > Thanks for the feedback.
> > 
> > Speaking of this issue isn't there a bug that if you do a bunch of
> > mprotects after mmap but before ECREATE kref_puts will underflow?
> > Unrelated to this patch.
> > 
> > Just quickly skimmed but I might be ignoring something..
> 
> kref_puts won't be called because vm_private_data will be null in the
> VMAs created by mprotect.  The ECREATE call would also fail as
> sgx_ioc_enclave_create checks to ensure the VMA it finds exactly
> matches the size of the enclave.

Right. Had forgot that I added that check at some point just because
of this issue :-) I think I'll add a comment about this in an updated
patch. Thank you.

/Jarkko
diff mbox

Patch

diff --git a/drivers/platform/x86/intel_sgx_main.c b/drivers/platform/x86/intel_sgx_main.c
index 93dd708..b5e414a 100644
--- a/drivers/platform/x86/intel_sgx_main.c
+++ b/drivers/platform/x86/intel_sgx_main.c
@@ -103,11 +103,8 @@  long sgx_compat_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	vma->vm_ops = &sgx_vm_ops;
-#if !defined(VM_RESERVED)
-	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO;
-#else
-	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_RESERVED | VM_IO;
-#endif
+	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO |
+			 VM_DONTCOPY;
 
 	return 0;
 }
diff --git a/drivers/platform/x86/intel_sgx_vma.c b/drivers/platform/x86/intel_sgx_vma.c
index e670405..cba50c2 100644
--- a/drivers/platform/x86/intel_sgx_vma.c
+++ b/drivers/platform/x86/intel_sgx_vma.c
@@ -71,42 +71,17 @@ 
 static void sgx_vma_open(struct vm_area_struct *vma)
 {
 	struct sgx_encl *encl = vma->vm_private_data;
-
-	/* When forking for the second time vm_private_data is already set to
-	 * NULL.
-	 */
-	if (!encl) {
-		zap_vma_ptes(vma, vma->vm_start, vma->vm_end - vma->vm_start);
-		return;
-	}
-
-	/* Invalidate enclave when the process has been forked. */
-	mutex_lock(&encl->lock);
-	if (encl->mm != vma->vm_mm) {
-		sgx_invalidate(encl);
-		vma->vm_private_data = NULL;
-	}
-	mutex_unlock(&encl->lock);
-
-	if (vma->vm_private_data)
-		kref_get(&encl->refcount);
+	kref_get(&encl->refcount);
 }
 
 static void sgx_vma_close(struct vm_area_struct *vma)
 {
 	struct sgx_encl *encl = vma->vm_private_data;
 
-	/* When forking for the second time vm_private_data is already set to
-	 * NULL.
-	 */
-	if (!encl)
-		return;
-
 	mutex_lock(&encl->lock);
 	zap_vma_ptes(vma, vma->vm_start, vma->vm_end - vma->vm_start);
 	encl->flags |= SGX_ENCL_DEAD;
 	mutex_unlock(&encl->lock);
-
 	kref_put(&encl->refcount, sgx_encl_release);
 }