| Message ID | 1490993459-15450-1-git-send-email-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 31, 2017 at 01:50:59PM -0700, Sean Christopherson wrote: > Update the EPC page tracking immediately after sgx_eldu, and retry > vm_insert_pfn on a future fault if vm_insert_pfn fails. Previously > we tried to EREMOVE the EPC page if vm_insert_pfn return an error, > but EREMOVE fails if there are active cpus in the enclave, in which > case the driver would effectively lose track of the EPC page. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> Right, for EREMOVE also the current epoch matters. > --- > drivers/platform/x86/intel_sgx.h | 1 + > drivers/platform/x86/intel_sgx_util.c | 41 +++++++++++++++++++++++++++-------- > 2 files changed, 33 insertions(+), 9 deletions(-) > > diff --git a/drivers/platform/x86/intel_sgx.h b/drivers/platform/x86/intel_sgx.h > index adb5b17..8b14b1f 100644 > --- a/drivers/platform/x86/intel_sgx.h > +++ b/drivers/platform/x86/intel_sgx.h > @@ -106,6 +106,7 @@ static inline void sgx_free_va_slot(struct sgx_va_page *page, > enum sgx_encl_page_flags { > SGX_ENCL_PAGE_TCS = BIT(0), > SGX_ENCL_PAGE_RESERVED = BIT(1), > + SGX_ENCL_PAGE_PTE_VALID = BIT(2), Yeah, this is a good idea. > }; > > struct sgx_encl_page { > diff --git a/drivers/platform/x86/intel_sgx_util.c b/drivers/platform/x86/intel_sgx_util.c > index 234a5fb..f4bbd38 100644 > --- a/drivers/platform/x86/intel_sgx_util.c > +++ b/drivers/platform/x86/intel_sgx_util.c > @@ -289,6 +289,22 @@ static int sgx_eldu(struct sgx_encl *encl, > return ret; > } > > +static inline int sgx_fault_insert_pfn(struct vm_area_struct *vma, > + struct sgx_encl *encl, > + struct sgx_encl_page *entry) > +{ > + int ret = 0; > + if (!(entry->flags & SGX_ENCL_PAGE_PTE_VALID)) { > + ret = vm_insert_pfn(vma, entry->addr, PFN_DOWN(entry->epc_page->pa)); > + if (!ret) { > + entry->flags |= SGX_ENCL_PAGE_PTE_VALID; > + sgx_test_and_clear_young(entry, encl); > + } > + } > + > + return ret; > +} > + > static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, > unsigned long addr, unsigned int flags) > { > @@ -340,6 +356,9 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, > > /* Legal race condition, page is already faulted. */ > if (entry->epc_page) { > + rc = sgx_fault_insert_pfn(vma, encl, entry); > + if (rc) > + goto out; Is this here to handle the condition where earlier caller loaded the page but sgx_fault_insert_pfn() failed? Why this is a better choice than killing the enclave? > if (reserve) > entry->flags |= SGX_ENCL_PAGE_RESERVED; > goto out; > @@ -369,22 +388,26 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, > if (rc) > goto out; > > - rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa)); > - if (rc) > - goto out; > - > + /* Track the page, even if vm_insert_pfn fails. We can't EREMOVE > + * the page because EREMOVE may fail due to an active cpu in the > + * enclave. We can't call vm_insert_pfn before sgx_eldu because > + * SKL platforms signal #GP instead of #PF if the EPC page is invalid. > + */ > encl->secs_child_cnt++; > > entry->epc_page = epc_page; > - > - if (reserve) > - entry->flags |= SGX_ENCL_PAGE_RESERVED; > + entry->flags &= ~SGX_ENCL_PAGE_PTE_VALID; > > /* Do not free */ > epc_page = NULL; > - > - sgx_test_and_clear_young(entry, encl); > list_add_tail(&entry->load_list, &encl->load_list); > + > + rc = sgx_fault_insert_pfn(vma, encl, entry); > + if (rc) > + goto out; > + > + if (reserve) > + entry->flags |= SGX_ENCL_PAGE_RESERVED; > out: > mutex_unlock(&encl->lock); > if (epc_page) > -- > 2.7.4 > > _______________________________________________ > intel-sgx-kernel-dev mailing list > intel-sgx-kernel-dev@lists.01.org > https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev /Jarkko
On Tue, Apr 04, 2017 at 08:01:03PM +0300, Jarkko Sakkinen wrote: > On Fri, Mar 31, 2017 at 01:50:59PM -0700, Sean Christopherson wrote: > > Update the EPC page tracking immediately after sgx_eldu, and retry > > vm_insert_pfn on a future fault if vm_insert_pfn fails. Previously > > we tried to EREMOVE the EPC page if vm_insert_pfn return an error, > > but EREMOVE fails if there are active cpus in the enclave, in which > > case the driver would effectively lose track of the EPC page. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > Right, for EREMOVE also the current epoch matters. > > > --- > > drivers/platform/x86/intel_sgx.h | 1 + > > drivers/platform/x86/intel_sgx_util.c | 41 +++++++++++++++++++++++++++-------- > > 2 files changed, 33 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/platform/x86/intel_sgx.h b/drivers/platform/x86/intel_sgx.h > > index adb5b17..8b14b1f 100644 > > --- a/drivers/platform/x86/intel_sgx.h > > +++ b/drivers/platform/x86/intel_sgx.h > > @@ -106,6 +106,7 @@ static inline void sgx_free_va_slot(struct sgx_va_page *page, > > enum sgx_encl_page_flags { > > SGX_ENCL_PAGE_TCS = BIT(0), > > SGX_ENCL_PAGE_RESERVED = BIT(1), > > + SGX_ENCL_PAGE_PTE_VALID = BIT(2), > > Yeah, this is a good idea. > > > }; > > > > struct sgx_encl_page { > > diff --git a/drivers/platform/x86/intel_sgx_util.c b/drivers/platform/x86/intel_sgx_util.c > > index 234a5fb..f4bbd38 100644 > > --- a/drivers/platform/x86/intel_sgx_util.c > > +++ b/drivers/platform/x86/intel_sgx_util.c > > @@ -289,6 +289,22 @@ static int sgx_eldu(struct sgx_encl *encl, > > return ret; > > } > > > > +static inline int sgx_fault_insert_pfn(struct vm_area_struct *vma, > > + struct sgx_encl *encl, > > + struct sgx_encl_page *entry) > > +{ > > + int ret = 0; > > + if (!(entry->flags & SGX_ENCL_PAGE_PTE_VALID)) { > > + ret = vm_insert_pfn(vma, entry->addr, PFN_DOWN(entry->epc_page->pa)); > > + if (!ret) { > > + entry->flags |= SGX_ENCL_PAGE_PTE_VALID; > > + sgx_test_and_clear_young(entry, encl); > > + } > > + } > > + > > + return ret; > > +} > > + > > static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, > > unsigned long addr, unsigned int flags) > > { > > @@ -340,6 +356,9 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, > > > > /* Legal race condition, page is already faulted. */ > > if (entry->epc_page) { > > + rc = sgx_fault_insert_pfn(vma, encl, entry); > > + if (rc) > > + goto out; > > Is this here to handle the condition where earlier caller loaded > the page but sgx_fault_insert_pfn() failed? > > Why this is a better choice than killing the enclave? I squashed everything but this commit that you've recently submitted. And also this is in the top of my master branch. If you can provide reasoning for the fallback, I'll squash this too. I've also tested all the commits. Thank you. /Jarkko
On Wed, Apr 05, 2017 at 11:04:40AM +0300, Jarkko Sakkinen wrote: > On Tue, Apr 04, 2017 at 08:01:03PM +0300, Jarkko Sakkinen wrote: > > On Fri, Mar 31, 2017 at 01:50:59PM -0700, Sean Christopherson wrote: > > > Update the EPC page tracking immediately after sgx_eldu, and retry > > > vm_insert_pfn on a future fault if vm_insert_pfn fails. Previously > > > we tried to EREMOVE the EPC page if vm_insert_pfn return an error, > > > but EREMOVE fails if there are active cpus in the enclave, in which > > > case the driver would effectively lose track of the EPC page. > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > > Right, for EREMOVE also the current epoch matters. > > > > > --- > > > drivers/platform/x86/intel_sgx.h | 1 + > > > drivers/platform/x86/intel_sgx_util.c | 41 +++++++++++++++++++++++++++-------- > > > 2 files changed, 33 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/platform/x86/intel_sgx.h b/drivers/platform/x86/intel_sgx.h > > > index adb5b17..8b14b1f 100644 > > > --- a/drivers/platform/x86/intel_sgx.h > > > +++ b/drivers/platform/x86/intel_sgx.h > > > @@ -106,6 +106,7 @@ static inline void sgx_free_va_slot(struct sgx_va_page *page, > > > enum sgx_encl_page_flags { > > > SGX_ENCL_PAGE_TCS = BIT(0), > > > SGX_ENCL_PAGE_RESERVED = BIT(1), > > > + SGX_ENCL_PAGE_PTE_VALID = BIT(2), > > > > Yeah, this is a good idea. > > > > > }; > > > > > > struct sgx_encl_page { > > > diff --git a/drivers/platform/x86/intel_sgx_util.c b/drivers/platform/x86/intel_sgx_util.c > > > index 234a5fb..f4bbd38 100644 > > > --- a/drivers/platform/x86/intel_sgx_util.c > > > +++ b/drivers/platform/x86/intel_sgx_util.c > > > @@ -289,6 +289,22 @@ static int sgx_eldu(struct sgx_encl *encl, > > > return ret; > > > } > > > > > > +static inline int sgx_fault_insert_pfn(struct vm_area_struct *vma, > > > + struct sgx_encl *encl, > > > + struct sgx_encl_page *entry) > > > +{ > > > + int ret = 0; > > > + if (!(entry->flags & SGX_ENCL_PAGE_PTE_VALID)) { > > > + ret = vm_insert_pfn(vma, entry->addr, PFN_DOWN(entry->epc_page->pa)); > > > + if (!ret) { > > > + entry->flags |= SGX_ENCL_PAGE_PTE_VALID; > > > + sgx_test_and_clear_young(entry, encl); > > > + } > > > + } > > > + > > > + return ret; > > > +} > > > + > > > static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, > > > unsigned long addr, unsigned int flags) > > > { > > > @@ -340,6 +356,9 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, > > > > > > /* Legal race condition, page is already faulted. */ > > > if (entry->epc_page) { > > > + rc = sgx_fault_insert_pfn(vma, encl, entry); > > > + if (rc) > > > + goto out; > > > > Is this here to handle the condition where earlier caller loaded > > the page but sgx_fault_insert_pfn() failed? > > > > Why this is a better choice than killing the enclave? > > I squashed everything but this commit that you've recently submitted. > > And also this is in the top of my master branch. If you can provide > reasoning for the fallback, I'll squash this too. > > I've also tested all the commits. Thank you. > > /Jarkko I'm dropping this commit for now as I need to refactor stuff to drivers/platform/x86/intel_sgx directory (in order to easily deploy LE later on). /Jarkko
On Mon, Apr 10, 2017 at 02:10:36PM +0300, Jarkko Sakkinen wrote: > On Wed, Apr 05, 2017 at 11:04:40AM +0300, Jarkko Sakkinen wrote: > > On Tue, Apr 04, 2017 at 08:01:03PM +0300, Jarkko Sakkinen wrote: > > > On Fri, Mar 31, 2017 at 01:50:59PM -0700, Sean Christopherson wrote: > > > > Update the EPC page tracking immediately after sgx_eldu, and retry > > > > vm_insert_pfn on a future fault if vm_insert_pfn fails. Previously > > > > we tried to EREMOVE the EPC page if vm_insert_pfn return an error, > > > > but EREMOVE fails if there are active cpus in the enclave, in which > > > > case the driver would effectively lose track of the EPC page. > > > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > > > > Right, for EREMOVE also the current epoch matters. > > > > > > > --- > > > > drivers/platform/x86/intel_sgx.h | 1 + > > > > drivers/platform/x86/intel_sgx_util.c | 41 +++++++++++++++++++++++++++-------- > > > > 2 files changed, 33 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/drivers/platform/x86/intel_sgx.h b/drivers/platform/x86/intel_sgx.h > > > > index adb5b17..8b14b1f 100644 > > > > --- a/drivers/platform/x86/intel_sgx.h > > > > +++ b/drivers/platform/x86/intel_sgx.h > > > > @@ -106,6 +106,7 @@ static inline void sgx_free_va_slot(struct sgx_va_page *page, > > > > enum sgx_encl_page_flags { > > > > SGX_ENCL_PAGE_TCS = BIT(0), > > > > SGX_ENCL_PAGE_RESERVED = BIT(1), > > > > + SGX_ENCL_PAGE_PTE_VALID = BIT(2), > > > > > > Yeah, this is a good idea. > > > > > > > }; > > > > > > > > struct sgx_encl_page { > > > > diff --git a/drivers/platform/x86/intel_sgx_util.c b/drivers/platform/x86/intel_sgx_util.c > > > > index 234a5fb..f4bbd38 100644 > > > > --- a/drivers/platform/x86/intel_sgx_util.c > > > > +++ b/drivers/platform/x86/intel_sgx_util.c > > > > @@ -289,6 +289,22 @@ static int sgx_eldu(struct sgx_encl *encl, > > > > return ret; > > > > } > > > > > > > > +static inline int sgx_fault_insert_pfn(struct vm_area_struct *vma, > > > > + struct sgx_encl *encl, > > > > + struct sgx_encl_page *entry) > > > > +{ > > > > + int ret = 0; > > > > + if (!(entry->flags & SGX_ENCL_PAGE_PTE_VALID)) { > > > > + ret = vm_insert_pfn(vma, entry->addr, PFN_DOWN(entry->epc_page->pa)); > > > > + if (!ret) { > > > > + entry->flags |= SGX_ENCL_PAGE_PTE_VALID; > > > > + sgx_test_and_clear_young(entry, encl); > > > > + } > > > > + } > > > > + > > > > + return ret; > > > > +} > > > > + > > > > static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, > > > > unsigned long addr, unsigned int flags) > > > > { > > > > @@ -340,6 +356,9 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, > > > > > > > > /* Legal race condition, page is already faulted. */ > > > > if (entry->epc_page) { > > > > + rc = sgx_fault_insert_pfn(vma, encl, entry); > > > > + if (rc) > > > > + goto out; > > > > > > Is this here to handle the condition where earlier caller loaded > > > the page but sgx_fault_insert_pfn() failed? > > > > > > Why this is a better choice than killing the enclave? > > > > I squashed everything but this commit that you've recently submitted. > > > > And also this is in the top of my master branch. If you can provide > > reasoning for the fallback, I'll squash this too. > > > > I've also tested all the commits. Thank you. > > > > /Jarkko > > I'm dropping this commit for now as I need to refactor stuff to > drivers/platform/x86/intel_sgx directory (in order to easily > deploy LE later on). > > /Jarkko I put a note on my backlog that this needs to be fixed so that it is not forgotten but it's right now in the way as I cannot squash it before refactoring the directory structure. /Jarkko
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote: > On Fri, Mar 31, 2017 at 01:50:59PM -0700, Sean Christopherson wrote: > > Update the EPC page tracking immediately after sgx_eldu, and retry > > vm_insert_pfn on a future fault if vm_insert_pfn fails. Previously > > we tried to EREMOVE the EPC page if vm_insert_pfn return an error, > > but EREMOVE fails if there are active cpus in the enclave, in which > > case the driver would effectively lose track of the EPC page. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > Right, for EREMOVE also the current epoch matters. > > > --- > > drivers/platform/x86/intel_sgx.h | 1 + > > drivers/platform/x86/intel_sgx_util.c | 41 > > +++++++++++++++++++++++++++-------- 2 files changed, 33 insertions(+), 9 > > deletions(-) > > > > diff --git a/drivers/platform/x86/intel_sgx.h > > b/drivers/platform/x86/intel_sgx.h index adb5b17..8b14b1f 100644 > > --- a/drivers/platform/x86/intel_sgx.h > > +++ b/drivers/platform/x86/intel_sgx.h > > @@ -106,6 +106,7 @@ static inline void sgx_free_va_slot(struct sgx_va_page > > *page, enum sgx_encl_page_flags { > > SGX_ENCL_PAGE_TCS = BIT(0), > > SGX_ENCL_PAGE_RESERVED = BIT(1), > > + SGX_ENCL_PAGE_PTE_VALID = BIT(2), > > Yeah, this is a good idea. > > > }; > > > > struct sgx_encl_page { > > diff --git a/drivers/platform/x86/intel_sgx_util.c > > b/drivers/platform/x86/intel_sgx_util.c index 234a5fb..f4bbd38 100644 > > --- a/drivers/platform/x86/intel_sgx_util.c > > +++ b/drivers/platform/x86/intel_sgx_util.c > > @@ -289,6 +289,22 @@ static int sgx_eldu(struct sgx_encl *encl, > > return ret; > > } > > > > +static inline int sgx_fault_insert_pfn(struct vm_area_struct *vma, > > + struct sgx_encl *encl, > > + struct sgx_encl_page *entry) > > +{ > > + int ret = 0; > > + if (!(entry->flags & SGX_ENCL_PAGE_PTE_VALID)) { > > + ret = vm_insert_pfn(vma, entry->addr, > > PFN_DOWN(entry->epc_page->pa)); > > + if (!ret) { > > + entry->flags |= SGX_ENCL_PAGE_PTE_VALID; > > + sgx_test_and_clear_young(entry, encl); > > + } > > + } > > + > > + return ret; > > +} > > + > > static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, > > unsigned long addr, unsigned int > > flags) { > > @@ -340,6 +356,9 @@ static struct sgx_encl_page *sgx_do_fault(struct > > vm_area_struct *vma, > > /* Legal race condition, page is already faulted. */ > > if (entry->epc_page) { > > + rc = sgx_fault_insert_pfn(vma, encl, entry); > > + if (rc) > > + goto out; > > Is this here to handle the condition where earlier caller loaded > the page but sgx_fault_insert_pfn() failed? Yes, the approach is to retry vm_insert_pfn if it failed after ELDU. > Why this is a better choice than killing the enclave? After digging deeper into vm_insert_pfn, it probably isn't a better option. I initially thought that there were transient conditions that would cause vm_insert_pfn to fail, but I don't think that is true. Killing the enclave is the correct approach if we get EFAULT or ENOMEM, and EBUSY is returned only if the PTE is already valid, which shouldn't occur with the SGX driver as PTE modification is protected by the enclave's lock. I'll work on a new patch to kill the enclave if vm_insert_pfn fails in the fault handler and to ensure the driver doesn't lose track of an EPC page. > > if (reserve) > > entry->flags |= SGX_ENCL_PAGE_RESERVED; > > goto out; > > @@ -369,22 +388,26 @@ static struct sgx_encl_page *sgx_do_fault(struct > > vm_area_struct *vma, if (rc) > > goto out; > > > > - rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa)); > > - if (rc) > > - goto out; > > - > > + /* Track the page, even if vm_insert_pfn fails. We can't EREMOVE > > + * the page because EREMOVE may fail due to an active cpu in the > > + * enclave. We can't call vm_insert_pfn before sgx_eldu because > > + * SKL platforms signal #GP instead of #PF if the EPC page is > > invalid. > > + */ > > encl->secs_child_cnt++; > > > > entry->epc_page = epc_page; > > - > > - if (reserve) > > - entry->flags |= SGX_ENCL_PAGE_RESERVED; > > + entry->flags &= ~SGX_ENCL_PAGE_PTE_VALID; > > > > /* Do not free */ > > epc_page = NULL; > > - > > - sgx_test_and_clear_young(entry, encl); > > list_add_tail(&entry->load_list, &encl->load_list); > > + > > + rc = sgx_fault_insert_pfn(vma, encl, entry); > > + if (rc) > > + goto out; > > + > > + if (reserve) > > + entry->flags |= SGX_ENCL_PAGE_RESERVED; > > out: > > mutex_unlock(&encl->lock); > > if (epc_page) > > -- > > 2.7.4 > > > > _______________________________________________ > > intel-sgx-kernel-dev mailing list > > intel-sgx-kernel-dev@lists.01.org > > https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev > > /Jarkko
On Mon, Apr 10, 2017 at 05:02:28PM +0000, Christopherson, Sean J wrote: > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote: > > On Fri, Mar 31, 2017 at 01:50:59PM -0700, Sean Christopherson wrote: > > > Update the EPC page tracking immediately after sgx_eldu, and retry > > > vm_insert_pfn on a future fault if vm_insert_pfn fails. Previously > > > we tried to EREMOVE the EPC page if vm_insert_pfn return an error, > > > but EREMOVE fails if there are active cpus in the enclave, in which > > > case the driver would effectively lose track of the EPC page. > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > > Right, for EREMOVE also the current epoch matters. > > > > > --- > > > drivers/platform/x86/intel_sgx.h | 1 + > > > drivers/platform/x86/intel_sgx_util.c | 41 > > > +++++++++++++++++++++++++++-------- 2 files changed, 33 insertions(+), 9 > > > deletions(-) > > > > > > diff --git a/drivers/platform/x86/intel_sgx.h > > > b/drivers/platform/x86/intel_sgx.h index adb5b17..8b14b1f 100644 > > > --- a/drivers/platform/x86/intel_sgx.h > > > +++ b/drivers/platform/x86/intel_sgx.h > > > @@ -106,6 +106,7 @@ static inline void sgx_free_va_slot(struct sgx_va_page > > > *page, enum sgx_encl_page_flags { > > > SGX_ENCL_PAGE_TCS = BIT(0), > > > SGX_ENCL_PAGE_RESERVED = BIT(1), > > > + SGX_ENCL_PAGE_PTE_VALID = BIT(2), > > > > Yeah, this is a good idea. > > > > > }; > > > > > > struct sgx_encl_page { > > > diff --git a/drivers/platform/x86/intel_sgx_util.c > > > b/drivers/platform/x86/intel_sgx_util.c index 234a5fb..f4bbd38 100644 > > > --- a/drivers/platform/x86/intel_sgx_util.c > > > +++ b/drivers/platform/x86/intel_sgx_util.c > > > @@ -289,6 +289,22 @@ static int sgx_eldu(struct sgx_encl *encl, > > > return ret; > > > } > > > > > > +static inline int sgx_fault_insert_pfn(struct vm_area_struct *vma, > > > + struct sgx_encl *encl, > > > + struct sgx_encl_page *entry) > > > +{ > > > + int ret = 0; > > > + if (!(entry->flags & SGX_ENCL_PAGE_PTE_VALID)) { > > > + ret = vm_insert_pfn(vma, entry->addr, > > > PFN_DOWN(entry->epc_page->pa)); > > > + if (!ret) { > > > + entry->flags |= SGX_ENCL_PAGE_PTE_VALID; > > > + sgx_test_and_clear_young(entry, encl); > > > + } > > > + } > > > + > > > + return ret; > > > +} > > > + > > > static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, > > > unsigned long addr, unsigned int > > > flags) { > > > @@ -340,6 +356,9 @@ static struct sgx_encl_page *sgx_do_fault(struct > > > vm_area_struct *vma, > > > /* Legal race condition, page is already faulted. */ > > > if (entry->epc_page) { > > > + rc = sgx_fault_insert_pfn(vma, encl, entry); > > > + if (rc) > > > + goto out; > > > > Is this here to handle the condition where earlier caller loaded > > the page but sgx_fault_insert_pfn() failed? > > Yes, the approach is to retry vm_insert_pfn if it failed after ELDU. > > > > Why this is a better choice than killing the enclave? > > After digging deeper into vm_insert_pfn, it probably isn't a better option. > I initially thought that there were transient conditions that would cause > vm_insert_pfn to fail, but I don't think that is true. Killing the enclave > is the correct approach if we get EFAULT or ENOMEM, and EBUSY is returned > only if the PTE is already valid, which shouldn't occur with the SGX driver > as PTE modification is protected by the enclave's lock. I'll work on a new > patch to kill the enclave if vm_insert_pfn fails in the fault handler and > to ensure the driver doesn't lose track of an EPC page. Could you send one more update on this patch? I apologize for directory structure change but it was necessary. The way LE is melded in has a lot of similarities how we did arch/x86/realmode/rm. Lots of cruft is coming and special makefiles so it does not make sense to have all that rooted to drivers/platform/x86 anymore. /Jarkko
diff --git a/drivers/platform/x86/intel_sgx.h b/drivers/platform/x86/intel_sgx.h index adb5b17..8b14b1f 100644 --- a/drivers/platform/x86/intel_sgx.h +++ b/drivers/platform/x86/intel_sgx.h @@ -106,6 +106,7 @@ static inline void sgx_free_va_slot(struct sgx_va_page *page, enum sgx_encl_page_flags { SGX_ENCL_PAGE_TCS = BIT(0), SGX_ENCL_PAGE_RESERVED = BIT(1), + SGX_ENCL_PAGE_PTE_VALID = BIT(2), }; struct sgx_encl_page { diff --git a/drivers/platform/x86/intel_sgx_util.c b/drivers/platform/x86/intel_sgx_util.c index 234a5fb..f4bbd38 100644 --- a/drivers/platform/x86/intel_sgx_util.c +++ b/drivers/platform/x86/intel_sgx_util.c @@ -289,6 +289,22 @@ static int sgx_eldu(struct sgx_encl *encl, return ret; } +static inline int sgx_fault_insert_pfn(struct vm_area_struct *vma, + struct sgx_encl *encl, + struct sgx_encl_page *entry) +{ + int ret = 0; + if (!(entry->flags & SGX_ENCL_PAGE_PTE_VALID)) { + ret = vm_insert_pfn(vma, entry->addr, PFN_DOWN(entry->epc_page->pa)); + if (!ret) { + entry->flags |= SGX_ENCL_PAGE_PTE_VALID; + sgx_test_and_clear_young(entry, encl); + } + } + + return ret; +} + static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, unsigned long addr, unsigned int flags) { @@ -340,6 +356,9 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, /* Legal race condition, page is already faulted. */ if (entry->epc_page) { + rc = sgx_fault_insert_pfn(vma, encl, entry); + if (rc) + goto out; if (reserve) entry->flags |= SGX_ENCL_PAGE_RESERVED; goto out; @@ -369,22 +388,26 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, if (rc) goto out; - rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa)); - if (rc) - goto out; - + /* Track the page, even if vm_insert_pfn fails. We can't EREMOVE + * the page because EREMOVE may fail due to an active cpu in the + * enclave. We can't call vm_insert_pfn before sgx_eldu because + * SKL platforms signal #GP instead of #PF if the EPC page is invalid. + */ encl->secs_child_cnt++; entry->epc_page = epc_page; - - if (reserve) - entry->flags |= SGX_ENCL_PAGE_RESERVED; + entry->flags &= ~SGX_ENCL_PAGE_PTE_VALID; /* Do not free */ epc_page = NULL; - - sgx_test_and_clear_young(entry, encl); list_add_tail(&entry->load_list, &encl->load_list); + + rc = sgx_fault_insert_pfn(vma, encl, entry); + if (rc) + goto out; + + if (reserve) + entry->flags |= SGX_ENCL_PAGE_RESERVED; out: mutex_unlock(&encl->lock); if (epc_page)
Update the EPC page tracking immediately after sgx_eldu, and retry vm_insert_pfn on a future fault if vm_insert_pfn fails. Previously we tried to EREMOVE the EPC page if vm_insert_pfn return an error, but EREMOVE fails if there are active cpus in the enclave, in which case the driver would effectively lose track of the EPC page. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- drivers/platform/x86/intel_sgx.h | 1 + drivers/platform/x86/intel_sgx_util.c | 41 +++++++++++++++++++++++++++-------- 2 files changed, 33 insertions(+), 9 deletions(-)