| Message ID | 37306EFA9975BE469F115FDE982C075B9B69682D@ORSMSX108.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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;
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;