| Message ID | 1491937510-11937-1-git-send-email-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 11, 2017 at 12:05:10PM -0700, Sean Christopherson wrote: > Update the EPC page tracking immediately after sgx_eldu and kill the > encl if vm_insert_pfn fails. Previously we tried to EREMOVE the EPC > page if vm_insert_pfn returned an error, but EREMOVE fails if there > are active cpus in the enclave, in which case the driver would lose > track of the EPC page. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> Thanks this makes a lot more sense. /Jarkko > --- > drivers/platform/x86/intel_sgx/sgx.h | 3 ++- > drivers/platform/x86/intel_sgx/sgx_main.c | 2 +- > drivers/platform/x86/intel_sgx/sgx_page_cache.c | 18 +++---------- > drivers/platform/x86/intel_sgx/sgx_util.c | 36 ++++++++++++++++++++----- > 4 files changed, 36 insertions(+), 23 deletions(-) > > diff --git a/drivers/platform/x86/intel_sgx/sgx.h b/drivers/platform/x86/intel_sgx/sgx.h > index 82355bb..e59ff01 100644 > --- a/drivers/platform/x86/intel_sgx/sgx.h > +++ b/drivers/platform/x86/intel_sgx/sgx.h > @@ -198,7 +198,8 @@ int sgx_eremove(struct sgx_epc_page *epc_page); > struct vm_area_struct *sgx_find_vma(struct sgx_encl *encl, unsigned long addr); > void sgx_zap_tcs_ptes(struct sgx_encl *encl, > struct vm_area_struct *vma); > -void sgx_invalidate(struct sgx_encl *encl); > +void sgx_invalidate(struct sgx_encl *encl, bool flush_cpus); > +void sgx_flush_cpus(struct sgx_encl *encl); > int sgx_find_encl(struct mm_struct *mm, unsigned long addr, > struct vm_area_struct **vma); > > diff --git a/drivers/platform/x86/intel_sgx/sgx_main.c b/drivers/platform/x86/intel_sgx/sgx_main.c > index b2a1bc4..5890643 100644 > --- a/drivers/platform/x86/intel_sgx/sgx_main.c > +++ b/drivers/platform/x86/intel_sgx/sgx_main.c > @@ -243,7 +243,7 @@ static int sgx_pm_suspend(struct device *dev) > > list_for_each_entry(ctx, &sgx_tgid_ctx_list, list) { > list_for_each_entry(encl, &ctx->encl_list, encl_list) { > - sgx_invalidate(encl); > + sgx_invalidate(encl, false); > encl->flags |= SGX_ENCL_SUSPEND; > flush_work(&encl->add_page_work); > } > diff --git a/drivers/platform/x86/intel_sgx/sgx_page_cache.c b/drivers/platform/x86/intel_sgx/sgx_page_cache.c > index 4ffde27..59a67cb 100644 > --- a/drivers/platform/x86/intel_sgx/sgx_page_cache.c > +++ b/drivers/platform/x86/intel_sgx/sgx_page_cache.c > @@ -221,15 +221,6 @@ static void sgx_isolate_pages(struct sgx_encl *encl, > mutex_unlock(&encl->lock); > } > > -static void sgx_ipi_cb(void *info) > -{ > -} > - > -static void sgx_flush_cpus(struct sgx_encl *encl) > -{ > - on_each_cpu_mask(mm_cpumask(encl->mm), sgx_ipi_cb, NULL, 1); > -} > - > static void sgx_eblock(struct sgx_encl *encl, > struct sgx_epc_page *epc_page) > { > @@ -242,8 +233,7 @@ static void sgx_eblock(struct sgx_encl *encl, > > if (ret) { > sgx_crit(encl, "EBLOCK returned %d\n", ret); > - sgx_invalidate(encl); > - sgx_flush_cpus(encl); > + sgx_invalidate(encl, true); > } > > } > @@ -259,8 +249,7 @@ static void sgx_etrack(struct sgx_encl *encl) > > if (ret) { > sgx_crit(encl, "ETRACK returned %d\n", ret); > - sgx_invalidate(encl); > - sgx_flush_cpus(encl); > + sgx_invalidate(encl, true); > } > } > > @@ -327,8 +316,7 @@ static bool sgx_ewb(struct sgx_encl *encl, > > if (ret) { > /* make enclave inaccessible */ > - sgx_invalidate(encl); > - sgx_flush_cpus(encl); > + sgx_invalidate(encl, true); > if (ret > 0) > sgx_err(encl, "EWB returned %d, enclave killed\n", ret); > return false; > diff --git a/drivers/platform/x86/intel_sgx/sgx_util.c b/drivers/platform/x86/intel_sgx/sgx_util.c > index 25e949d..1e9fa18 100644 > --- a/drivers/platform/x86/intel_sgx/sgx_util.c > +++ b/drivers/platform/x86/intel_sgx/sgx_util.c > @@ -120,7 +120,7 @@ void sgx_zap_tcs_ptes(struct sgx_encl *encl, struct vm_area_struct *vma) > } > } > > -void sgx_invalidate(struct sgx_encl *encl) > +void sgx_invalidate(struct sgx_encl *encl, bool flush_cpus) > { > struct vm_area_struct *vma; > unsigned long addr; > @@ -135,6 +135,18 @@ void sgx_invalidate(struct sgx_encl *encl) > } > > encl->flags |= SGX_ENCL_DEAD; > + > + if (flush_cpus) > + sgx_flush_cpus(encl); > +} > + > +static void sgx_ipi_cb(void *info) > +{ > +} > + > +void sgx_flush_cpus(struct sgx_encl *encl) > +{ > + on_each_cpu_mask(mm_cpumask(encl->mm), sgx_ipi_cb, NULL, 1); > } > > /** > @@ -315,10 +327,12 @@ 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 EPC page even if vm_insert_pfn fails; we need to ensure > + * the EPC page is properly freed and we can't do EREMOVE right away > + * because EREMOVE may fail due to an active cpu in the enclave. We > + * can't call vm_insert_pfn before sgx_eldu because SKL signals #GP > + * instead of #PF if the EPC page is invalid. > + */ > encl->secs_child_cnt++; > > entry->epc_page = epc_page; > @@ -328,9 +342,19 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, > > /* Do not free */ > epc_page = NULL; > + list_add_tail(&entry->load_list, &encl->load_list); > + > + rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(entry->epc_page->pa)); > + if (rc) { > + /* Kill the enclave if vm_insert_pfn fails; failure only occurs > + * if there is a driver bug or an unrecoverable issue, e.g. OOM. > + */ > + sgx_crit(encl, "vm_insert_pfn returned %d\n", rc); > + sgx_invalidate(encl, true); > + goto out; > + } > > sgx_test_and_clear_young(entry, encl); > - list_add_tail(&entry->load_list, &encl->load_list); > 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
On Tue, Apr 11, 2017 at 12:05:10PM -0700, Sean Christopherson wrote: > Update the EPC page tracking immediately after sgx_eldu and kill the > encl if vm_insert_pfn fails. Previously we tried to EREMOVE the EPC > page if vm_insert_pfn returned an error, but EREMOVE fails if there > are active cpus in the enclave, in which case the driver would lose > track of the EPC page. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> Thanks. Applied > --- > drivers/platform/x86/intel_sgx/sgx.h | 3 ++- > drivers/platform/x86/intel_sgx/sgx_main.c | 2 +- > drivers/platform/x86/intel_sgx/sgx_page_cache.c | 18 +++---------- > drivers/platform/x86/intel_sgx/sgx_util.c | 36 ++++++++++++++++++++----- > 4 files changed, 36 insertions(+), 23 deletions(-) > > diff --git a/drivers/platform/x86/intel_sgx/sgx.h b/drivers/platform/x86/intel_sgx/sgx.h > index 82355bb..e59ff01 100644 > --- a/drivers/platform/x86/intel_sgx/sgx.h > +++ b/drivers/platform/x86/intel_sgx/sgx.h > @@ -198,7 +198,8 @@ int sgx_eremove(struct sgx_epc_page *epc_page); > struct vm_area_struct *sgx_find_vma(struct sgx_encl *encl, unsigned long addr); > void sgx_zap_tcs_ptes(struct sgx_encl *encl, > struct vm_area_struct *vma); > -void sgx_invalidate(struct sgx_encl *encl); > +void sgx_invalidate(struct sgx_encl *encl, bool flush_cpus); > +void sgx_flush_cpus(struct sgx_encl *encl); > int sgx_find_encl(struct mm_struct *mm, unsigned long addr, > struct vm_area_struct **vma); > > diff --git a/drivers/platform/x86/intel_sgx/sgx_main.c b/drivers/platform/x86/intel_sgx/sgx_main.c > index b2a1bc4..5890643 100644 > --- a/drivers/platform/x86/intel_sgx/sgx_main.c > +++ b/drivers/platform/x86/intel_sgx/sgx_main.c > @@ -243,7 +243,7 @@ static int sgx_pm_suspend(struct device *dev) > > list_for_each_entry(ctx, &sgx_tgid_ctx_list, list) { > list_for_each_entry(encl, &ctx->encl_list, encl_list) { > - sgx_invalidate(encl); > + sgx_invalidate(encl, false); > encl->flags |= SGX_ENCL_SUSPEND; > flush_work(&encl->add_page_work); > } > diff --git a/drivers/platform/x86/intel_sgx/sgx_page_cache.c b/drivers/platform/x86/intel_sgx/sgx_page_cache.c > index 4ffde27..59a67cb 100644 > --- a/drivers/platform/x86/intel_sgx/sgx_page_cache.c > +++ b/drivers/platform/x86/intel_sgx/sgx_page_cache.c > @@ -221,15 +221,6 @@ static void sgx_isolate_pages(struct sgx_encl *encl, > mutex_unlock(&encl->lock); > } > > -static void sgx_ipi_cb(void *info) > -{ > -} > - > -static void sgx_flush_cpus(struct sgx_encl *encl) > -{ > - on_each_cpu_mask(mm_cpumask(encl->mm), sgx_ipi_cb, NULL, 1); > -} > - > static void sgx_eblock(struct sgx_encl *encl, > struct sgx_epc_page *epc_page) > { > @@ -242,8 +233,7 @@ static void sgx_eblock(struct sgx_encl *encl, > > if (ret) { > sgx_crit(encl, "EBLOCK returned %d\n", ret); > - sgx_invalidate(encl); > - sgx_flush_cpus(encl); > + sgx_invalidate(encl, true); > } > > } > @@ -259,8 +249,7 @@ static void sgx_etrack(struct sgx_encl *encl) > > if (ret) { > sgx_crit(encl, "ETRACK returned %d\n", ret); > - sgx_invalidate(encl); > - sgx_flush_cpus(encl); > + sgx_invalidate(encl, true); > } > } > > @@ -327,8 +316,7 @@ static bool sgx_ewb(struct sgx_encl *encl, > > if (ret) { > /* make enclave inaccessible */ > - sgx_invalidate(encl); > - sgx_flush_cpus(encl); > + sgx_invalidate(encl, true); > if (ret > 0) > sgx_err(encl, "EWB returned %d, enclave killed\n", ret); > return false; > diff --git a/drivers/platform/x86/intel_sgx/sgx_util.c b/drivers/platform/x86/intel_sgx/sgx_util.c > index 25e949d..1e9fa18 100644 > --- a/drivers/platform/x86/intel_sgx/sgx_util.c > +++ b/drivers/platform/x86/intel_sgx/sgx_util.c > @@ -120,7 +120,7 @@ void sgx_zap_tcs_ptes(struct sgx_encl *encl, struct vm_area_struct *vma) > } > } > > -void sgx_invalidate(struct sgx_encl *encl) > +void sgx_invalidate(struct sgx_encl *encl, bool flush_cpus) > { > struct vm_area_struct *vma; > unsigned long addr; > @@ -135,6 +135,18 @@ void sgx_invalidate(struct sgx_encl *encl) > } > > encl->flags |= SGX_ENCL_DEAD; > + > + if (flush_cpus) > + sgx_flush_cpus(encl); > +} > + > +static void sgx_ipi_cb(void *info) > +{ > +} > + > +void sgx_flush_cpus(struct sgx_encl *encl) > +{ > + on_each_cpu_mask(mm_cpumask(encl->mm), sgx_ipi_cb, NULL, 1); > } > > /** > @@ -315,10 +327,12 @@ 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 EPC page even if vm_insert_pfn fails; we need to ensure > + * the EPC page is properly freed and we can't do EREMOVE right away > + * because EREMOVE may fail due to an active cpu in the enclave. We > + * can't call vm_insert_pfn before sgx_eldu because SKL signals #GP > + * instead of #PF if the EPC page is invalid. > + */ > encl->secs_child_cnt++; > > entry->epc_page = epc_page; > @@ -328,9 +342,19 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, > > /* Do not free */ > epc_page = NULL; > + list_add_tail(&entry->load_list, &encl->load_list); > + > + rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(entry->epc_page->pa)); > + if (rc) { > + /* Kill the enclave if vm_insert_pfn fails; failure only occurs > + * if there is a driver bug or an unrecoverable issue, e.g. OOM. > + */ > + sgx_crit(encl, "vm_insert_pfn returned %d\n", rc); > + sgx_invalidate(encl, true); > + goto out; > + } > > sgx_test_and_clear_young(entry, encl); > - list_add_tail(&entry->load_list, &encl->load_list); > 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
diff --git a/drivers/platform/x86/intel_sgx/sgx.h b/drivers/platform/x86/intel_sgx/sgx.h index 82355bb..e59ff01 100644 --- a/drivers/platform/x86/intel_sgx/sgx.h +++ b/drivers/platform/x86/intel_sgx/sgx.h @@ -198,7 +198,8 @@ int sgx_eremove(struct sgx_epc_page *epc_page); struct vm_area_struct *sgx_find_vma(struct sgx_encl *encl, unsigned long addr); void sgx_zap_tcs_ptes(struct sgx_encl *encl, struct vm_area_struct *vma); -void sgx_invalidate(struct sgx_encl *encl); +void sgx_invalidate(struct sgx_encl *encl, bool flush_cpus); +void sgx_flush_cpus(struct sgx_encl *encl); int sgx_find_encl(struct mm_struct *mm, unsigned long addr, struct vm_area_struct **vma); diff --git a/drivers/platform/x86/intel_sgx/sgx_main.c b/drivers/platform/x86/intel_sgx/sgx_main.c index b2a1bc4..5890643 100644 --- a/drivers/platform/x86/intel_sgx/sgx_main.c +++ b/drivers/platform/x86/intel_sgx/sgx_main.c @@ -243,7 +243,7 @@ static int sgx_pm_suspend(struct device *dev) list_for_each_entry(ctx, &sgx_tgid_ctx_list, list) { list_for_each_entry(encl, &ctx->encl_list, encl_list) { - sgx_invalidate(encl); + sgx_invalidate(encl, false); encl->flags |= SGX_ENCL_SUSPEND; flush_work(&encl->add_page_work); } diff --git a/drivers/platform/x86/intel_sgx/sgx_page_cache.c b/drivers/platform/x86/intel_sgx/sgx_page_cache.c index 4ffde27..59a67cb 100644 --- a/drivers/platform/x86/intel_sgx/sgx_page_cache.c +++ b/drivers/platform/x86/intel_sgx/sgx_page_cache.c @@ -221,15 +221,6 @@ static void sgx_isolate_pages(struct sgx_encl *encl, mutex_unlock(&encl->lock); } -static void sgx_ipi_cb(void *info) -{ -} - -static void sgx_flush_cpus(struct sgx_encl *encl) -{ - on_each_cpu_mask(mm_cpumask(encl->mm), sgx_ipi_cb, NULL, 1); -} - static void sgx_eblock(struct sgx_encl *encl, struct sgx_epc_page *epc_page) { @@ -242,8 +233,7 @@ static void sgx_eblock(struct sgx_encl *encl, if (ret) { sgx_crit(encl, "EBLOCK returned %d\n", ret); - sgx_invalidate(encl); - sgx_flush_cpus(encl); + sgx_invalidate(encl, true); } } @@ -259,8 +249,7 @@ static void sgx_etrack(struct sgx_encl *encl) if (ret) { sgx_crit(encl, "ETRACK returned %d\n", ret); - sgx_invalidate(encl); - sgx_flush_cpus(encl); + sgx_invalidate(encl, true); } } @@ -327,8 +316,7 @@ static bool sgx_ewb(struct sgx_encl *encl, if (ret) { /* make enclave inaccessible */ - sgx_invalidate(encl); - sgx_flush_cpus(encl); + sgx_invalidate(encl, true); if (ret > 0) sgx_err(encl, "EWB returned %d, enclave killed\n", ret); return false; diff --git a/drivers/platform/x86/intel_sgx/sgx_util.c b/drivers/platform/x86/intel_sgx/sgx_util.c index 25e949d..1e9fa18 100644 --- a/drivers/platform/x86/intel_sgx/sgx_util.c +++ b/drivers/platform/x86/intel_sgx/sgx_util.c @@ -120,7 +120,7 @@ void sgx_zap_tcs_ptes(struct sgx_encl *encl, struct vm_area_struct *vma) } } -void sgx_invalidate(struct sgx_encl *encl) +void sgx_invalidate(struct sgx_encl *encl, bool flush_cpus) { struct vm_area_struct *vma; unsigned long addr; @@ -135,6 +135,18 @@ void sgx_invalidate(struct sgx_encl *encl) } encl->flags |= SGX_ENCL_DEAD; + + if (flush_cpus) + sgx_flush_cpus(encl); +} + +static void sgx_ipi_cb(void *info) +{ +} + +void sgx_flush_cpus(struct sgx_encl *encl) +{ + on_each_cpu_mask(mm_cpumask(encl->mm), sgx_ipi_cb, NULL, 1); } /** @@ -315,10 +327,12 @@ 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 EPC page even if vm_insert_pfn fails; we need to ensure + * the EPC page is properly freed and we can't do EREMOVE right away + * because EREMOVE may fail due to an active cpu in the enclave. We + * can't call vm_insert_pfn before sgx_eldu because SKL signals #GP + * instead of #PF if the EPC page is invalid. + */ encl->secs_child_cnt++; entry->epc_page = epc_page; @@ -328,9 +342,19 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, /* Do not free */ epc_page = NULL; + list_add_tail(&entry->load_list, &encl->load_list); + + rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(entry->epc_page->pa)); + if (rc) { + /* Kill the enclave if vm_insert_pfn fails; failure only occurs + * if there is a driver bug or an unrecoverable issue, e.g. OOM. + */ + sgx_crit(encl, "vm_insert_pfn returned %d\n", rc); + sgx_invalidate(encl, true); + goto out; + } sgx_test_and_clear_young(entry, encl); - list_add_tail(&entry->load_list, &encl->load_list); out: mutex_unlock(&encl->lock); if (epc_page)
Update the EPC page tracking immediately after sgx_eldu and kill the encl if vm_insert_pfn fails. Previously we tried to EREMOVE the EPC page if vm_insert_pfn returned an error, but EREMOVE fails if there are active cpus in the enclave, in which case the driver would lose track of the EPC page. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- drivers/platform/x86/intel_sgx/sgx.h | 3 ++- drivers/platform/x86/intel_sgx/sgx_main.c | 2 +- drivers/platform/x86/intel_sgx/sgx_page_cache.c | 18 +++---------- drivers/platform/x86/intel_sgx/sgx_util.c | 36 ++++++++++++++++++++----- 4 files changed, 36 insertions(+), 23 deletions(-)