x86/sgx: Fix ELDU MAC failures

Revert a change that directly propagates the page type from SECINFO into encl_page->desc. encl_page->desc is a packed value, jamming the SECINFO page_type sets bits that are intended for other uses.

Specifically, bits 11:3 are used to store the VA offset when a page is swapped out and are available for other uses when the page is resident in the EPC. To allow overloading bits 11:3, the VA offset is cleared at ELDU and so is not explicitly cleared at EWB, e.g. the VA offset is OR'd into encl_page->desc. As a result, a subsequent ELDU gets a MAC failure due to loading the wrong VA offset.

Signed-off-by Sean Christopherson <[email protected]>

---
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/x86/kernel/cpu/sgx/driver/ioctl.c +0 -0

index 85e36e530baf..355ce967a77f 100644

Quoted 21 lines · Sean Christopherson

Revert a change that directly propagates the page type from SECINFO into encl_page->desc. encl_page->desc is a packed value, jamming the SECINFO page_type sets bits that are intended for other uses.

Specifically, bits 11:3 are used to store the VA offset when a page is swapped out and are available for other uses when the page is resident in the EPC. To allow overloading bits 11:3, the VA offset is cleared at ELDU and so is not explicitly cleared at EWB, e.g. the VA offset is OR'd into encl_page->desc. As a result, a subsequent ELDU gets a MAC failure due to loading the wrong VA offset.

Signed-off-by Sean Christopherson <[email protected]>

---
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/x86/kernel/cpu/sgx/driver/ioctl.c +0 -0

index 85e36e530baf..355ce967a77f 100644

arch/x86/kernel/cpu/sgx/driver/ioctl.c static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl, · line 145

Quoted 7 lines

arch/x86/kernel/cpu/sgx/driver/ioctl.c +2 -1

@@ -140,7 +140,8 @@ static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
 		return ERR_PTR(-ENOMEM);
 
 	encl_page->desc = addr;
-	encl_page->desc |= page_type;
+	if (page_type == SGX_SECINFO_TCS)
+		encl_page->desc |= SGX_ENCL_PAGE_TCS;
 	encl_page->encl = encl;

Oops. How embrassing from my side. Please also ignore my original review comment about unreadability. The parameter type was right and everything was right. I was just blind for a while.

This is exactly how it should be e.g. no boolean parameter. I think with most of this kind of APIs we should follow a patttern the API takes PT and the function does whatever encoding we have.

Quoted 34 lines · Jarkko Sakkinen

Quoted 21 lines · Sean Christopherson

Revert a change that directly propagates the page type from SECINFO into encl_page->desc. encl_page->desc is a packed value, jamming the SECINFO page_type sets bits that are intended for other uses.

Specifically, bits 11:3 are used to store the VA offset when a page is swapped out and are available for other uses when the page is resident in the EPC. To allow overloading bits 11:3, the VA offset is cleared at ELDU and so is not explicitly cleared at EWB, e.g. the VA offset is OR'd into encl_page->desc. As a result, a subsequent ELDU gets a MAC failure due to loading the wrong VA offset.

Signed-off-by Sean Christopherson <[email protected]>

---
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/x86/kernel/cpu/sgx/driver/ioctl.c +0 -0

index 85e36e530baf..355ce967a77f 100644

arch/x86/kernel/cpu/sgx/driver/ioctl.c static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl, · line 145

Quoted 7 lines

arch/x86/kernel/cpu/sgx/driver/ioctl.c +2 -1

@@ -140,7 +140,8 @@ static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
 		return ERR_PTR(-ENOMEM);
 
 	encl_page->desc = addr;
-	encl_page->desc |= page_type;
+	if (page_type == SGX_SECINFO_TCS)
+		encl_page->desc |= SGX_ENCL_PAGE_TCS;
 	encl_page->encl = encl;

Oops. How embrassing from my side. Please also ignore my original review comment about unreadability. The parameter type was right and everything was right. I was just blind for a while.

Quoted 3 lines

This is exactly how it should be e.g. no boolean parameter. I think with most of this kind of APIs we should follow a patttern the API takes PT and the function does whatever encoding we have.

In any case the issue is fixed now on my tree and I also rewrote commit message for the driver.