[for_v22,01/11] x86/sgx: Fix an SECS collision with enclave page at VA=0
diff mbox series

Message ID 20190808001254.11926-2-sean.j.christopherson@intel.com
State New
Headers show
Series
  • x86/sgx: Bug fixes for v22
Related show

Commit Message

Sean Christopherson Aug. 8, 2019, 12:12 a.m. UTC
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(-)

Comments

Jarkko Sakkinen Aug. 8, 2019, 3:34 p.m. UTC | #1
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
Sean Christopherson Aug. 8, 2019, 3:44 p.m. UTC | #2
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?
Jarkko Sakkinen Aug. 9, 2019, 3:13 p.m. UTC | #3
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
Jarkko Sakkinen Aug. 9, 2019, 8:44 p.m. UTC | #4
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
Jarkko Sakkinen Aug. 9, 2019, 8:59 p.m. UTC | #5
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

Patch
diff mbox series

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);