x86/sgx: Sanitize pages in two rounds

Commit Message

Jarkko Sakkinen Feb. 6, 2020, 4:15 p.m. UTC

WARN_ONCE() can trigger after kexec() on systems with multiple EPC
sections. This can happen when an enclave has a SECS page in some section
and child pages in a section processed after the section containing the
SECS page. CPU does not allow to remove SECS before all of its children
have been removed.

Fix this by removing the tail from sgx_sanitize_section() and iterate
sections in two rounds by calling the resulting function.

Finally, report to the user space only after all processing has been
done and not in the middle of processing as before. This improves the
quality of reporting as kernel can tell how many unsanitized pages in
each section was left unprocessed.

Cc: [e-mail]
Reported-by: Sean Christopherson <[e-mail]>
Signed-off-by: Jarkko Sakkinen <[e-mail]>
---
This is a patch to fix the issue in the version 25 of the SGX patch set.

Apply against https://github.com/jsakkine-intel/linux-sgx.git.
 arch/x86/kernel/cpu/sgx/reclaim.c | 38 ++++++++++++++-----------------
 1 file changed, 17 insertions(+), 21 deletions(-)

Comments

Jarkko Sakkinen Feb. 9, 2020, 9:04 p.m. UTC | #1

On Thu, Feb 06, 2020 at 06:15:50PM +0200, Jarkko Sakkinen wrote:
> WARN_ONCE() can trigger after kexec() on systems with multiple EPC
> sections. This can happen when an enclave has a SECS page in some section
> and child pages in a section processed after the section containing the
> SECS page. CPU does not allow to remove SECS before all of its children
> have been removed.
> 
> Fix this by removing the tail from sgx_sanitize_section() and iterate
> sections in two rounds by calling the resulting function.
> 
> Finally, report to the user space only after all processing has been
> done and not in the middle of processing as before. This improves the
> quality of reporting as kernel can tell how many unsanitized pages in
> each section was left unprocessed.
> 
> Cc: [e-mail]
> Reported-by: Sean Christopherson <[e-mail]>
> Signed-off-by: Jarkko Sakkinen <[e-mail]>

Merged.

/Jarkko

Patch

diff --git a/arch/x86/kernel/cpu/sgx/reclaim.c b/arch/x86/kernel/cpu/sgx/reclaim.c
index a33f1c45477a..5a88e1c388b5 100644
--- a/arch/x86/kernel/cpu/sgx/reclaim.c
+++ b/arch/x86/kernel/cpu/sgx/reclaim.c
@@ -18,10 +18,6 @@  DECLARE_WAIT_QUEUE_HEAD(ksgxswapd_waitq);
 LIST_HEAD(sgx_active_page_list);
 DEFINE_SPINLOCK(sgx_active_page_list_lock);
 
-/*
- * Reset all pages to uninitialized state. Pages could be in initialized on
- * kmemexec.
- */
 static void sgx_sanitize_section(struct sgx_epc_section *section)
 {
 	struct sgx_epc_page *page, *tmp;
@@ -47,23 +43,6 @@  static void sgx_sanitize_section(struct sgx_epc_section *section)
 
 		cond_resched();
 	}
-
-	list_for_each_entry_safe(page, tmp, &secs_list, list) {
-		if (kthread_should_stop())
-			return;
-
-		ret = __eremove(sgx_epc_addr(page));
-		if (!WARN_ON_ONCE(ret)) {
-			spin_lock(&section->lock);
-			list_move(&page->list, &section->page_list);
-			spin_unlock(&section->lock);
-		} else {
-			list_del(&page->list);
-			kfree(page);
-		}
-
-		cond_resched();
-	}
 }
 
 static int ksgxswapd(void *p)
@@ -72,9 +51,26 @@  static int ksgxswapd(void *p)
 
 	set_freezable();
 
+	/*
+	 * Reset all pages to uninitialized state. Pages could be in initialized
+	 * on kmemexec.
+	 */
+	for (i = 0; i < sgx_nr_epc_sections; i++)
+		sgx_sanitize_section(&sgx_epc_sections[i]);
+
+	/*
+	 * 2nd round for the SECS pages as they cannot be removed when they
+	 * still hold child pages.
+	 */
 	for (i = 0; i < sgx_nr_epc_sections; i++)
 		sgx_sanitize_section(&sgx_epc_sections[i]);
 
+	/* Check that all pages were removed. */
+	for (i = 0; i < sgx_nr_epc_sections; i++) {
+		if (!list_empty(&sgx_epc_sections[i].unsanitized_page_list))
+			WARN(1, "EPC section %d has unsanitized pages.\n", i);
+	}
+
 	while (!kthread_should_stop()) {
 		if (try_to_freeze())
 			continue;