| Message ID | 1483477647-12054-3-git-send-email-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 03, 2017 at 01:07:26PM -0800, Sean Christopherson wrote: > Swap the order of the calls to vm_insert_pfn and do_eldu to make ELDU > the last action in the fault handling sequence, which eleminates the > need to do EREMOVE of the page if vm_insert_pfn fails. EREMOVE fails > if there are active threads in the enclave, i.e. the previous code > could result in kernel panics due to EREMOVE failure. > > Update the return value if vm_insert_pfn fails in order to capture > vm_insert_pfn's error code. > > Inserting the page before ELDU does not create a race condition > as accesses to the page will still #PF due to failing the EPCM > checks, i.e. user-visible behavior is identical whether an access > faults due to an invalid PTE or an invalid EPCM entry. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> NAK. I did not even read the code because the short description says "bug fixes". A single patch per bug fix please. /Jarkko > --- > drivers/platform/x86/intel_sgx_vma.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/platform/x86/intel_sgx_vma.c b/drivers/platform/x86/intel_sgx_vma.c > index e8a67b6..f356eed 100644 > --- a/drivers/platform/x86/intel_sgx_vma.c > +++ b/drivers/platform/x86/intel_sgx_vma.c > @@ -160,7 +160,6 @@ static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma, > struct sgx_epc_page *epc_page = NULL; > struct sgx_epc_page *secs_epc_page = NULL; > struct page *backing; > - unsigned int free_flags = SGX_FREE_SKIP_EREMOVE; > int rc; > > /* If process was forked, VMA is still there but vm_private_data is set > @@ -243,18 +242,19 @@ static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma, > goto out; > } > > - rc = do_eldu(encl, entry, epc_page, backing, false /* is_secs */); > + rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa)); > if (rc) { > sgx_put_backing(backing, 0); > entry = ERR_PTR(rc); > goto out; > } > > - rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa)); > + rc = do_eldu(encl, entry, epc_page, backing, false /* is_secs */); > sgx_put_backing(backing, 0); > > if (rc) { > - free_flags = 0; > + zap_vma_ptes(vma, entry->addr, PAGE_SIZE); > + entry = ERR_PTR(rc); > goto out; > } > > @@ -273,7 +273,7 @@ static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma, > out: > mutex_unlock(&encl->lock); > if (epc_page) > - sgx_free_page(epc_page, encl, free_flags); > + sgx_free_page(epc_page, encl, SGX_FREE_SKIP_EREMOVE); > if (secs_epc_page) > sgx_free_page(secs_epc_page, encl, SGX_FREE_SKIP_EREMOVE); > return entry; > -- > 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_vma.c b/drivers/platform/x86/intel_sgx_vma.c index e8a67b6..f356eed 100644 --- a/drivers/platform/x86/intel_sgx_vma.c +++ b/drivers/platform/x86/intel_sgx_vma.c @@ -160,7 +160,6 @@ static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma, struct sgx_epc_page *epc_page = NULL; struct sgx_epc_page *secs_epc_page = NULL; struct page *backing; - unsigned int free_flags = SGX_FREE_SKIP_EREMOVE; int rc; /* If process was forked, VMA is still there but vm_private_data is set @@ -243,18 +242,19 @@ static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma, goto out; } - rc = do_eldu(encl, entry, epc_page, backing, false /* is_secs */); + rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa)); if (rc) { sgx_put_backing(backing, 0); entry = ERR_PTR(rc); goto out; } - rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa)); + rc = do_eldu(encl, entry, epc_page, backing, false /* is_secs */); sgx_put_backing(backing, 0); if (rc) { - free_flags = 0; + zap_vma_ptes(vma, entry->addr, PAGE_SIZE); + entry = ERR_PTR(rc); goto out; } @@ -273,7 +273,7 @@ static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma, out: mutex_unlock(&encl->lock); if (epc_page) - sgx_free_page(epc_page, encl, free_flags); + sgx_free_page(epc_page, encl, SGX_FREE_SKIP_EREMOVE); if (secs_epc_page) sgx_free_page(secs_epc_page, encl, SGX_FREE_SKIP_EREMOVE); return entry;
Swap the order of the calls to vm_insert_pfn and do_eldu to make ELDU the last action in the fault handling sequence, which eleminates the need to do EREMOVE of the page if vm_insert_pfn fails. EREMOVE fails if there are active threads in the enclave, i.e. the previous code could result in kernel panics due to EREMOVE failure. Update the return value if vm_insert_pfn fails in order to capture vm_insert_pfn's error code. Inserting the page before ELDU does not create a race condition as accesses to the page will still #PF due to failing the EPCM checks, i.e. user-visible behavior is identical whether an access faults due to an invalid PTE or an invalid EPCM entry. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- drivers/platform/x86/intel_sgx_vma.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)