| Message ID | 20190808001254.11926-2-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Bug fixes for v22 | expand |
On Wed, Aug 07, 2019 at 05:12:44PM -0700, Sean Christopherson wrote: > Detect the SECS in paging related flows by explicitly checking the page > against the enclave's SECS page. Assuming a page with VA=0 is the SECS > will break enclaves that actually use VA=0, which is extremely unlikely > but theoretically possible. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> I would define a macro to the same place where SGX_ENCL_PAGE_ADDR() is defined and also SGX_ENCL_PAGE_IS_SECS() would definitely more self-describing name. Can't you BTW just use the backpointer in struct sgx_encl_page to the enclave since we have it there? It is even set for SECS in sgx_encl_create(). Also, lets try to avoid VA acronym in SGX context for other than version array. I had a brief moment of confusion when reading the commit message :-) /Jarkko
On Thu, Aug 08, 2019 at 06:34:59PM +0300, Jarkko Sakkinen wrote: > On Wed, Aug 07, 2019 at 05:12:44PM -0700, Sean Christopherson wrote: > > Detect the SECS in paging related flows by explicitly checking the page > > against the enclave's SECS page. Assuming a page with VA=0 is the SECS > > will break enclaves that actually use VA=0, which is extremely unlikely > > but theoretically possible. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > I would define a macro to the same place where SGX_ENCL_PAGE_ADDR() is > defined and also SGX_ENCL_PAGE_IS_SECS() would definitely more > self-describing name. > > Can't you BTW just use the backpointer in struct sgx_encl_page to the > enclave since we have it there? It is even set for SECS in > sgx_encl_create(). Yeah, that would work too. I passed in @encl to match the format of sgx_encl_get_index(), perhaps it makes sense to use the backpointer there as well?
On Thu, 2019-08-08 at 08:44 -0700, Sean Christopherson wrote: > On Thu, Aug 08, 2019 at 06:34:59PM +0300, Jarkko Sakkinen wrote: > > On Wed, Aug 07, 2019 at 05:12:44PM -0700, Sean Christopherson wrote: > > > Detect the SECS in paging related flows by explicitly checking the page > > > against the enclave's SECS page. Assuming a page with VA=0 is the SECS > > > will break enclaves that actually use VA=0, which is extremely unlikely > > > but theoretically possible. > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > > I would define a macro to the same place where SGX_ENCL_PAGE_ADDR() is > > defined and also SGX_ENCL_PAGE_IS_SECS() would definitely more > > self-describing name. > > > > Can't you BTW just use the backpointer in struct sgx_encl_page to the > > enclave since we have it there? It is even set for SECS in > > sgx_encl_create(). > > Yeah, that would work too. I passed in @encl to match the format of > sgx_encl_get_index(), perhaps it makes sense to use the backpointer there > as well? Yes, it does of course. Probably have just forgotten to add it. This kind of inconsistencies exist because backpointer has not been always existing. /Jarkko
On Wed, Aug 07, 2019 at 05:12:44PM -0700, Sean Christopherson wrote: > Detect the SECS in paging related flows by explicitly checking the page > against the enclave's SECS page. Assuming a page with VA=0 is the SECS > will break enclaves that actually use VA=0, which is extremely unlikely > but theoretically possible. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> I applied this with the tweaks mentioned in the discussion: * SGX_ENCL_PAGE_IS_SECS() as a macro. * Removed @encl both sgx_get_index() and SGX_ENCL_PAGE_IS_SECS(). Not yet pushed. Just noting that I'm taking care of it. /Jarkko
On Fri, Aug 09, 2019 at 11:44:56PM +0300, Jarkko Sakkinen wrote: > On Wed, Aug 07, 2019 at 05:12:44PM -0700, Sean Christopherson wrote: > > Detect the SECS in paging related flows by explicitly checking the page > > against the enclave's SECS page. Assuming a page with VA=0 is the SECS > > will break enclaves that actually use VA=0, which is extremely unlikely > > but theoretically possible. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > I applied this with the tweaks mentioned in the discussion: > > * SGX_ENCL_PAGE_IS_SECS() as a macro. > * Removed @encl both sgx_get_index() and SGX_ENCL_PAGE_IS_SECS(). > > Not yet pushed. Just noting that I'm taking care of it. Not it is also pushed. /Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index 909af9a664f0..6da1c36a01e6 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -12,10 +12,14 @@ #include "encls.h" #include "sgx.h" +static bool sgx_encl_is_secs(struct sgx_encl *encl, struct sgx_encl_page *page) +{ + return page == &encl->secs; +} + static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, struct sgx_epc_page *epc_page) { - unsigned long addr = SGX_ENCL_PAGE_ADDR(encl_page); unsigned long va_offset = SGX_ENCL_PAGE_VA_OFFSET(encl_page); struct sgx_encl *encl = encl_page->encl; pgoff_t page_index = sgx_encl_get_index(encl, encl_page); @@ -38,11 +42,11 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, goto err_pcmd; } - pginfo.addr = addr; + pginfo.addr = SGX_ENCL_PAGE_ADDR(encl_page); pginfo.contents = (unsigned long)kmap_atomic(backing); pginfo.metadata = (unsigned long)kmap_atomic(pcmd) + pcmd_offset; - pginfo.secs = addr ? (unsigned long)sgx_epc_addr(encl->secs.epc_page) : - 0; + pginfo.secs = sgx_encl_is_secs(encl, encl_page) ? 0 : + (unsigned long)sgx_epc_addr(encl->secs.epc_page); ret = __eldu(&pginfo, sgx_epc_addr(epc_page), sgx_epc_addr(encl_page->va_page->epc_page) + va_offset); @@ -546,7 +550,7 @@ void sgx_encl_release(struct kref *ref) */ pgoff_t sgx_encl_get_index(struct sgx_encl *encl, struct sgx_encl_page *page) { - if (!PFN_DOWN(page->desc)) + if (sgx_encl_is_secs(encl, page)) return PFN_DOWN(encl->size); return PFN_DOWN(page->desc - encl->base);
Detect the SECS in paging related flows by explicitly checking the page against the enclave's SECS page. Assuming a page with VA=0 is the SECS will break enclaves that actually use VA=0, which is extremely unlikely but theoretically possible. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kernel/cpu/sgx/encl.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)