[intel-sgx-kernel-dev,v6,6/8] intel_sgx: disallow VMA reconfiguration after EPC pages have been added
diff mbox

Message ID 37306EFA9975BE469F115FDE982C075B9B69682D@ORSMSX108.amr.corp.intel.com
State New
Headers show

Commit Message

Sean Christopherson Dec. 5, 2016, 5:46 p.m. UTC
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
>> Do not allow VMA reconfiguration after EPC pages are added because SGX1
>> permissions are static. The policy might be easened with SGX2 (EMODP) but it
>> is better to start with this because in SGX1 the PTE permissions and EPCM
>> permissions must be in-sync.

This patch breaks user space.  The SDK adds all pages via the SGX ioctl before calling mprotect to set its paging permissions.  All code related to SGX_ENCL_PAGES_ADDED needs to be removed, the rest of the patch is good.  The commit message also needs to be reworked since the resulting patch is basically just replacing vma_cnt with SGX_ENCL_INVALIDATED; or maybe just merge this patch with the next patch, "intel_sgx: invalidate enclave when the user threads cease to exist".

e.g. apply this on top of the patch

Comments

Jarkko Sakkinen Dec. 6, 2016, 6:28 p.m. UTC | #1
On Mon, Dec 05, 2016 at 05:46:16PM +0000, Christopherson, Sean J wrote:
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> >> Do not allow VMA reconfiguration after EPC pages are added because SGX1
> >> permissions are static. The policy might be easened with SGX2 (EMODP) but it
> >> is better to start with this because in SGX1 the PTE permissions and EPCM
> >> permissions must be in-sync.
> 
> This patch breaks user space.  The SDK adds all pages via the SGX
> ioctl before calling mprotect to set its paging permissions.  All code
> related to SGX_ENCL_PAGES_ADDED needs to be removed, the rest of the
> patch is good.  The commit message also needs to be reworked since the
> resulting patch is basically just replacing vma_cnt with
> SGX_ENCL_INVALIDATED; or maybe just merge this patch with the next
> patch, "intel_sgx: invalidate enclave when the user threads cease to
> exist".
> 
> e.g. apply this on top of the patch

There's no stable driver API before the driver is in the mainline. By
definition breaking the user space is fine as long as you have valid
arguments to do that.

Allowing to change VMAs after adding pages is not right thing to do
because it makes the driver unstable.

/Jarkko

> 
> diff --git a/intel_sgx.h b/intel_sgx.h
> index 35c03fc..a891176 100644
> --- a/intel_sgx.h
> +++ b/intel_sgx.h
> @@ -130,8 +130,7 @@ enum sgx_encl_flags {
>         SGX_ENCL_DEBUG          = BIT(1),
>         SGX_ENCL_SECS_EVICTED   = BIT(2),
>         SGX_ENCL_SUSPEND        = BIT(3),
> -       SGX_ENCL_PAGES_ADDED    = BIT(4),
> -       SGX_ENCL_INVALIDATED    = BIT(5),
> +       SGX_ENCL_INVALIDATED    = BIT(4),
>  };
> 
>  struct sgx_encl {
> diff --git a/intel_sgx_ioctl.c b/intel_sgx_ioctl.c
> index 0c3fd29..6ab67ea 100644
> --- a/intel_sgx_ioctl.c
> +++ b/intel_sgx_ioctl.c
> @@ -736,7 +736,6 @@ out:
>         } else {
>                 ret = encl_rb_insert(&encl->encl_rb, encl_page);
>                 WARN_ON(ret);
> -               encl->flags |= SGX_ENCL_PAGES_ADDED;
>         }
> 
>         mutex_unlock(&encl->lock);
> diff --git a/intel_sgx_vma.c b/intel_sgx_vma.c
> index 4515cc3..517085d 100644
> --- a/intel_sgx_vma.c
> +++ b/intel_sgx_vma.c
> @@ -81,11 +81,10 @@ static void sgx_vma_open(struct vm_area_struct *vma)
>         }
> 
>         /* Invalidate enclave when the process has been forked for the first
> -        * time or pages have been added because PTEs must bee in sync with
> -        * the EPCM entries.
> +        * time.
>          */
>         mutex_lock(&encl->lock);
> -       if (encl->mm != vma->vm_mm || (encl->flags & SGX_ENCL_PAGES_ADDED)) {
> +       if (encl->mm != vma->vm_mm) {
>                 encl->flags |= SGX_ENCL_INVALIDATED;
>                 zap_vma_ptes(vma, vma->vm_start, vma->vm_end - vma->vm_start);
>                 vma->vm_private_data = NULL;

Patch
diff mbox

diff --git a/intel_sgx.h b/intel_sgx.h
index 35c03fc..a891176 100644
--- a/intel_sgx.h
+++ b/intel_sgx.h
@@ -130,8 +130,7 @@  enum sgx_encl_flags {
        SGX_ENCL_DEBUG          = BIT(1),
        SGX_ENCL_SECS_EVICTED   = BIT(2),
        SGX_ENCL_SUSPEND        = BIT(3),
-       SGX_ENCL_PAGES_ADDED    = BIT(4),
-       SGX_ENCL_INVALIDATED    = BIT(5),
+       SGX_ENCL_INVALIDATED    = BIT(4),
 };

 struct sgx_encl {
diff --git a/intel_sgx_ioctl.c b/intel_sgx_ioctl.c
index 0c3fd29..6ab67ea 100644
--- a/intel_sgx_ioctl.c
+++ b/intel_sgx_ioctl.c
@@ -736,7 +736,6 @@  out:
        } else {
                ret = encl_rb_insert(&encl->encl_rb, encl_page);
                WARN_ON(ret);
-               encl->flags |= SGX_ENCL_PAGES_ADDED;
        }

        mutex_unlock(&encl->lock);
diff --git a/intel_sgx_vma.c b/intel_sgx_vma.c
index 4515cc3..517085d 100644
--- a/intel_sgx_vma.c
+++ b/intel_sgx_vma.c
@@ -81,11 +81,10 @@  static void sgx_vma_open(struct vm_area_struct *vma)
        }

        /* Invalidate enclave when the process has been forked for the first
-        * time or pages have been added because PTEs must bee in sync with
-        * the EPCM entries.
+        * time.
         */
        mutex_lock(&encl->lock);
-       if (encl->mm != vma->vm_mm || (encl->flags & SGX_ENCL_PAGES_ADDED)) {
+       if (encl->mm != vma->vm_mm) {
                encl->flags |= SGX_ENCL_INVALIDATED;
                zap_vma_ptes(vma, vma->vm_start, vma->vm_end - vma->vm_start);
                vma->vm_private_data = NULL;