linux-kernel - [PATCH] x86/sgx: rename and document SGX bit lock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20210112221901.2CE31C8E@viggo.jf.intel.com>
Date:   Tue, 12 Jan 2021 14:19:01 -0800
From:   Dave Hansen <dave.hansen@...ux.intel.com>
To:     linux-kernel@...r.kernel.org
Cc:     Dave Hansen <dave.hansen@...ux.intel.com>,
        sean.j.christopherson@...el.com, jarkko@...nel.org, bp@...e.de,
        x86@...nel.org
Subject: [PATCH] x86/sgx: rename and document SGX bit lock


SGX ioctl() calls are serialized with a lock.  It's a weird open-coded
lock that is not even called a "lock".  That makes it a weird beast,
but Sean has convinced me it's a good idea without better alternatives.

Give the lock bit a better name, and document what it actually trying
to do.

Cc: Sean Christopherson <sean.j.christopherson@...el.com>
Cc: Jarkko Sakkinen <jarkko@...nel.org>
Cc: Borislav Petkov <bp@...e.de>
Cc: x86@...nel.org

---

 b/arch/x86/kernel/cpu/sgx/encl.h  |    2 +-
 b/arch/x86/kernel/cpu/sgx/ioctl.c |   19 ++++++++++++++++---
 2 files changed, 17 insertions(+), 4 deletions(-)

diff -puN arch/x86/kernel/cpu/sgx/ioctl.c~sgx-encl-flags arch/x86/kernel/cpu/sgx/ioctl.c
--- a/arch/x86/kernel/cpu/sgx/ioctl.c~sgx-encl-flags	2021-01-12 14:02:24.480689006 -0800
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c	2021-01-12 14:02:24.486689006 -0800
@@ -690,8 +690,21 @@ long sgx_ioctl(struct file *filep, unsig
 	struct sgx_encl *encl = filep->private_data;
 	int ret;
 
-	if (test_and_set_bit(SGX_ENCL_IOCTL, &encl->flags))
-		return -EBUSY;
+	/*
+	 * Behold, the Big SGX Lock
+	 *
+	 * The primary function of this "lock" is to actively discourage
+	 * attempts at multi-threaded enclave management.  Enclave management
+	 * is fundamentally a single-threaded affair.  Enclave measurement,
+	 * for instance would be worthless if two ADD_PAGES instances raced
+	 * and occurred in different orders.
+	 *
+	 * encl->lock is ill suited for this because it would need to be
+	 * conditionally dropped and reqacuired for operations like enclave
+	 * page allocation and reclaim.
+	 */
+	if (test_and_set_bit(SGX_ENCL_IOCTL_LOCK, &encl->flags))
+		return -EINVAL;
 
 	switch (cmd) {
 	case SGX_IOC_ENCLAVE_CREATE:
@@ -711,6 +724,6 @@ long sgx_ioctl(struct file *filep, unsig
 		break;
 	}
 
-	clear_bit(SGX_ENCL_IOCTL, &encl->flags);
+	clear_bit(SGX_ENCL_IOCTL_LOCK, &encl->flags);
 	return ret;
 }
diff -puN arch/x86/kernel/cpu/sgx/encl.h~sgx-encl-flags arch/x86/kernel/cpu/sgx/encl.h
--- a/arch/x86/kernel/cpu/sgx/encl.h~sgx-encl-flags	2021-01-12 14:02:24.482689006 -0800
+++ b/arch/x86/kernel/cpu/sgx/encl.h	2021-01-12 14:16:37.511686879 -0800
@@ -34,7 +34,7 @@ struct sgx_encl_page {
 };
 
 enum sgx_encl_flags {
-	SGX_ENCL_IOCTL		= BIT(0),
+	SGX_ENCL_IOCTL_LOCK	= BIT(0), /* See sgx_ioctl() */
 	SGX_ENCL_DEBUG		= BIT(1),
 	SGX_ENCL_CREATED	= BIT(2),
 	SGX_ENCL_INITIALIZED	= BIT(3),
_