On Thu, Jun 25, 2020 at 07:23:19PM +0200, Borislav Petkov wrote:

On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:

quoted

Intel Software Guard eXtensions (SGX) is a set of CPU instructions that
can be used by applications to set aside private regions of code and
data. The code outside the SGX hosted software entity is disallowed to
access the memory inside the enclave enforced by the CPU. We call these
entities as enclaves.

This commit implements a driver that provides an ioctl API to construct
and run enclaves. Enclaves are constructed from pages residing in
reserved physical memory areas. The contents of these pages can only be
accessed when they are mapped as part of an enclave, by a hardware
thread running inside the enclave.

The starting state of an enclave consists of a fixed measured set of
pages that are copied to the EPC during the construction process by
using ENCLS leaf functions and Software Enclave Control Structure (SECS)
that defines the enclave properties.

Enclave are constructed by using ENCLS leaf functions ECREATE, EADD and
EINIT. ECREATE initializes SECS, EADD copies pages from system memory to
the EPC and EINIT check a given signed measurement and moves the enclave
into a state ready for execution.

An initialized enclave can only be accessed through special Thread Control
Structure (TCS) pages by using ENCLU (ring-3 only) leaf EENTER.  This leaf
function converts a thread into enclave mode and continues the execution in
the offset defined by the TCS provided to EENTER. An enclave is exited
through syscall, exception, interrupts or by explicitly calling another
ENCLU leaf EEXIT.

The permissions, which enclave page is added will set the limit for maximum
permissions that can be set for mmap() and mprotect(). This will
effectively allow to build different security schemes between producers and
consumers of enclaves. Later on we can increase granularity with LSM hooks
for page addition (i.e. for producers) and mapping of the enclave (i.e. for
consumers)

Cc: linux-security-module@vger.kernel.org
Acked-by: Jethro Beekman <redacted>
Tested-by: Jethro Beekman <redacted>
Tested-by: Haitao Huang <redacted>
Tested-by: Chunyang Hui <redacted>
Tested-by: Jordan Hand <redacted>
Tested-by: Nathaniel McCallum <redacted>
Tested-by: Seth Moore <redacted>
Co-developed-by: Sean Christopherson <redacted>
Signed-off-by: Sean Christopherson <redacted>
Co-developed-by: Suresh Siddha <redacted>
Signed-off-by: Suresh Siddha <redacted>
Signed-off-by: Jarkko Sakkinen <redacted>
---
 .../userspace-api/ioctl/ioctl-number.rst      |   1 +
 arch/x86/include/uapi/asm/sgx.h               |  66 ++
 arch/x86/kernel/cpu/sgx/Makefile              |   3 +
 arch/x86/kernel/cpu/sgx/driver.c              | 194 +++++
 arch/x86/kernel/cpu/sgx/driver.h              |  30 +
 arch/x86/kernel/cpu/sgx/encl.c                | 335 +++++++++
 arch/x86/kernel/cpu/sgx/encl.h                |  87 +++
 arch/x86/kernel/cpu/sgx/ioctl.c               | 706 ++++++++++++++++++
 arch/x86/kernel/cpu/sgx/main.c                |  11 +
 9 files changed, 1433 insertions(+)
 create mode 100644 arch/x86/include/uapi/asm/sgx.h
 create mode 100644 arch/x86/kernel/cpu/sgx/driver.c
 create mode 100644 arch/x86/kernel/cpu/sgx/driver.h
 create mode 100644 arch/x86/kernel/cpu/sgx/encl.c
 create mode 100644 arch/x86/kernel/cpu/sgx/encl.h
 create mode 100644 arch/x86/kernel/cpu/sgx/ioctl.c

diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 59472cd6a11d..35f713e3a267 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst

@@ -323,6 +323,7 @@ Code  Seq#    Include File                                           Comments
                                                                      <mailto:tlewis@mindspring.com>
 0xA3  90-9F  linux/dtlk.h
 0xA4  00-1F  uapi/linux/tee.h                                        Generic TEE subsystem
+0xA4  00-1F  uapi/asm/sgx.h                                          Intel SGX subsystem (a legit conflict as TEE and SGX do not co-exist)
 0xAA  00-3F  linux/uapi/linux/userfaultfd.h
 0xAB  00-1F  linux/nbd.h
 0xAC  00-1F  linux/raw.h

diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
new file mode 100644
index 000000000000..5edb08ab8fd0
--- /dev/null
+++ b/arch/x86/include/uapi/asm/sgx.h

@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */

Checkpatch complains here:

WARNING: 'SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */' is not supported in LICENSES/...
#114: FILE: arch/x86/include/uapi/asm/sgx.h:1:
+/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */

Also, you had all patches until now split nice and logically doing one
thing only.

But this one is huge. Why?

Why can't you split out the facilities which the driver uses: encl.[ch]
into a patch, then ioctl.c into a separate one and then the driver into
a third one? Or do they all belong together inseparably?

I guess I'll find out eventually but it would've been nice if they were
split out...

It's still kind a strongly connected set of functionalities, but I get
your point.

I'd consider splitting for a slighly different angle:

1. Commit for the base driver.
2. Commit for each ioctl, adding the necessary "framework" to get that
   piece of functionality completed. The order would be:
   A. Create
   B. Add
   C. Initialize

Would be probably easier to review also this way because the commit kind
of rationalizes why things exist.

What do you think?

/Jarkko
 
 /Jarkko

/Jarkko