[PATCH v3 0/2] x86: sgx_vepc: implement ioctl to EREMOVE all pages

[PATCH v3 0/2] x86: sgx_vepc: implement ioctl to EREMOVE all pages

[Paolo Bonzini]

Add to /dev/sgx_vepc a ioctl that brings vEPC pages back to uninitialized
state with EREMOVE.  This is useful in order to match the expectations
of guests after reboot, and to match the behavior of real hardware.

The ioctl is a cleaner alternative to closing and reopening the
/dev/sgx_vepc device; reopening /dev/sgx_vepc could be problematic in
case userspace has sandboxed itself since the time it first opened the
device, and has thus lost permissions to do so.

If possible, I would like these patches to be included in 5.15 through
either the x86 or the KVM tree.

Thanks,

Paolo

Changes from RFC:
- improved commit messages, added documentation
- renamed ioctl from SGX_IOC_VEPC_REMOVE to SGX_IOC_VEPC_REMOVE_ALL

Change from v1:
- fixed documentation and code to cover SGX_ENCLAVE_ACT errors
- removed Tested-by since the code is quite different now

Changes from v2:
- return EBUSY also if EREMOVE causes a general protection fault

Paolo Bonzini (2):
  x86: sgx_vepc: extract sgx_vepc_remove_page
  x86: sgx_vepc: implement SGX_IOC_VEPC_REMOVE_ALL ioctl

 Documentation/x86/sgx.rst       | 35 +++++++++++++++++++++
 arch/x86/include/uapi/asm/sgx.h |  2 ++
 arch/x86/kernel/cpu/sgx/virt.c  | 63 ++++++++++++++++++++++++++++++---
 3 files changed, 95 insertions(+), 5 deletions(-)

-- 
2.27.0

[PATCH v3 1/2] x86: sgx_vepc: extract sgx_vepc_remove_page

[Paolo Bonzini]

For bare-metal SGX on real hardware, the hardware provides guarantees
SGX state at reboot.  For instance, all pages start out uninitialized.
The vepc driver provides a similar guarantee today for freshly-opened
vepc instances, but guests such as Windows expect all pages to be in
uninitialized state on startup, including after every guest reboot.

One way to do this is to simply close and reopen the /dev/sgx_vepc file
descriptor and re-mmap the virtual EPC.  However, this is problematic
because it prevents sandboxing the userspace (for example forbidding
open() after the guest starts; this is doable with heavy use of SCM_RIGHTS
file descriptor passing).

In order to implement this, we will need a ioctl that performs
EREMOVE on all pages mapped by a /dev/sgx_vepc file descriptor:
other possibilities, such as closing and reopening the device,
are racy.

Start the implementation by creating a separate function with just
the __eremove wrapper.

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kernel/cpu/sgx/virt.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
index 64511c4a5200..59cdf3f742ac 100644
--- a/arch/x86/kernel/cpu/sgx/virt.c
+++ b/arch/x86/kernel/cpu/sgx/virt.c
@@ -111,10 +111,8 @@ static int sgx_vepc_mmap(struct file *file, struct vm_area_struct *vma)
 	return 0;
 }
 
-static int sgx_vepc_free_page(struct sgx_epc_page *epc_page)
+static int sgx_vepc_remove_page(struct sgx_epc_page *epc_page)
 {
-	int ret;
-
 	/*
 	 * Take a previously guest-owned EPC page and return it to the
 	 * general EPC page pool.
@@ -124,7 +122,12 @@ static int sgx_vepc_free_page(struct sgx_epc_page *epc_page)
 	 * case that a guest properly EREMOVE'd this page, a superfluous
 	 * EREMOVE is harmless.
 	 */
-	ret = __eremove(sgx_get_epc_virt_addr(epc_page));
+	return __eremove(sgx_get_epc_virt_addr(epc_page));
+}
+
+static int sgx_vepc_free_page(struct sgx_epc_page *epc_page)
+{
+	int ret = sgx_vepc_remove_page(epc_page);
 	if (ret) {
 		/*
 		 * Only SGX_CHILD_PRESENT is expected, which is because of
@@ -144,7 +147,6 @@ static int sgx_vepc_free_page(struct sgx_epc_page *epc_page)
 	}
 
 	sgx_free_epc_page(epc_page);
-
 	return 0;
 }
 
-- 
2.27.0

[PATCH v3 2/2] x86: sgx_vepc: implement SGX_IOC_VEPC_REMOVE ioctl

[Paolo Bonzini]

For bare-metal SGX on real hardware, the hardware provides guarantees
SGX state at reboot.  For instance, all pages start out uninitialized.
The vepc driver provides a similar guarantee today for freshly-opened
vepc instances, but guests such as Windows expect all pages to be in
uninitialized state on startup, including after every guest reboot.

Some userspace implementations of virtual SGX would rather avoid having
to close and reopen the /dev/sgx_vepc file descriptor and re-mmap the
virtual EPC.  For example, they could sandbox themselves after the guest
starts and forbid further calls to open(), in order to mitigate exploits
from untrusted guests.

Therefore, add a ioctl that does this with EREMOVE.  Userspace can
invoke the ioctl to bring its vEPC pages back to uninitialized state.
There is a possibility that some pages fail to be removed if they are
SECS pages, and the child and SECS pages could be in separate vEPC
regions.  Therefore, the ioctl returns the number of EREMOVE failures,
telling userspace to try the ioctl again after it's done with all
vEPC regions.  A more verbose description of the correct usage and
the possible error conditions is documented in sgx.rst.

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	v2->v3: return EBUSY for a general protection fault

 Documentation/x86/sgx.rst       | 35 +++++++++++++++++++++
 arch/x86/include/uapi/asm/sgx.h |  2 ++
 arch/x86/kernel/cpu/sgx/virt.c  | 51 +++++++++++++++++++++++++++++++++
 3 files changed, 88 insertions(+)

diff --git a/Documentation/x86/sgx.rst b/Documentation/x86/sgx.rst
index dd0ac96ff9ef..7bc9c3b297d6 100644
--- a/Documentation/x86/sgx.rst
+++ b/Documentation/x86/sgx.rst
@@ -250,3 +250,38 @@ user wants to deploy SGX applications both on the host and in guests
 on the same machine, the user should reserve enough EPC (by taking out
 total virtual EPC size of all SGX VMs from the physical EPC size) for
 host SGX applications so they can run with acceptable performance.
+
+Architectural behavior is to restore all EPC pages to an uninitialized
+state also after a guest reboot.  Because this state can be reached only
+through the privileged ``ENCLS[EREMOVE]`` instruction, ``/dev/sgx_vepc``
+provides the ``SGX_IOC_VEPC_REMOVE_ALL`` ioctl to execute the instruction
+on all pages in the virtual EPC.
+
+``EREMOVE`` can fail for three reasons.  Userspace must pay attention
+to expected failures and handle them as follows:
+
+1. Page removal will always fail when any thread is running in the
+   enclave to which the page belongs.  In this case the ioctl will
+   return ``EBUSY`` independent of whether it has successfully removed
+   some pages; userspace can avoid these failures by preventing execution
+   of any vcpu which maps the virtual EPC.
+
+2. Page removal will cause a general protection fault if two calls to
+   ``EREMOVE`` happen concurrently for pages that refer to the same
+   "SECS" metadata pages.  This can happen if there are concurrent
+   invocations to ``SGX_IOC_VEPC_REMOVE_ALL``, or if a ``/dev/sgx_vepc``
+   file descriptor in the guest is closed at the same time as
+   ``SGX_IOC_VEPC_REMOVE_ALL``; it will also be reported as ``EBUSY``.
+   This can be avoided in userspace by serializing calls to the ioctl()
+   and to close(), but in general it should not be a problem.
+
+3. Finally, page removal will fail for SECS metadata pages which still
+   have child pages.  Child pages can be removed by executing
+   ``SGX_IOC_VEPC_REMOVE_ALL`` on all ``/dev/sgx_vepc`` file descriptors
+   mapped into the guest.  This means that the ioctl() must be called
+   twice: an initial set of calls to remove child pages and a subsequent
+   set of calls to remove SECS pages.  The second set of calls is only
+   required for those mappings that returned a nonzero value from the
+   first call.  It indicates a bug in the kernel or the userspace client
+   if any of the second round of ``SGX_IOC_VEPC_REMOVE_ALL`` calls has
+   a return code other than 0.
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 9690d6899ad9..f4b81587e90b 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -27,6 +27,8 @@ enum sgx_page_flags {
 	_IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
 #define SGX_IOC_ENCLAVE_PROVISION \
 	_IOW(SGX_MAGIC, 0x03, struct sgx_enclave_provision)
+#define SGX_IOC_VEPC_REMOVE_ALL \
+	_IO(SGX_MAGIC, 0x04)
 
 /**
  * struct sgx_enclave_create - parameter structure for the
diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
index 59cdf3f742ac..e8469bf68fd6 100644
--- a/arch/x86/kernel/cpu/sgx/virt.c
+++ b/arch/x86/kernel/cpu/sgx/virt.c
@@ -150,6 +150,39 @@ static int sgx_vepc_free_page(struct sgx_epc_page *epc_page)
 	return 0;
 }
 
+static long sgx_vepc_remove_all(struct sgx_vepc *vepc)
+{
+	struct sgx_epc_page *entry;
+	unsigned long index;
+	long failures = 0;
+
+	xa_for_each(&vepc->page_array, index, entry) {
+		int ret = sgx_vepc_remove_page(entry);
+		if (ret) {
+			if (ret == SGX_CHILD_PRESENT) {
+				failures++;
+			} else {
+				/*
+				 * Unlike in sgx_vepc_free_page, userspace might
+				 * call the ioctl while logical processors are
+				 * running in the enclave, or cause faults due
+				 * to concurrent access to pages under the same
+				 * SECS.  So we cannot warn, we just report it.
+				 */
+				return -EBUSY;
+			}
+		}
+		cond_resched();
+	}
+
+	/*
+	 * Return the number of pages that failed to be removed, so
+	 * userspace knows that there are still SECS pages lying
+	 * around.
+	 */
+	return failures;
+}
+
 static int sgx_vepc_release(struct inode *inode, struct file *file)
 {
 	struct sgx_vepc *vepc = file->private_data;
@@ -235,9 +268,27 @@ static int sgx_vepc_open(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static long sgx_vepc_ioctl(struct file *file,
+			   unsigned int cmd, unsigned long arg)
+{
+	struct sgx_vepc *vepc = file->private_data;
+
+	switch (cmd) {
+	case SGX_IOC_VEPC_REMOVE_ALL:
+		if (arg)
+			return -EINVAL;
+		return sgx_vepc_remove_all(vepc);
+
+	default:
+		return -ENOTTY;
+	}
+}
+
 static const struct file_operations sgx_vepc_fops = {
 	.owner		= THIS_MODULE,
 	.open		= sgx_vepc_open,
+	.unlocked_ioctl	= sgx_vepc_ioctl,
+	.compat_ioctl	= sgx_vepc_ioctl,
 	.release	= sgx_vepc_release,
 	.mmap		= sgx_vepc_mmap,
 };
-- 
2.27.0

Re: [PATCH v3 0/2] x86: sgx_vepc: implement ioctl to EREMOVE all pages

[Jarkko Sakkinen]

On Sat, 2021-10-16 at 03:14 -0400, Paolo Bonzini wrote:
> Add to /dev/sgx_vepc a ioctl that brings vEPC pages back to uninitialized
> state with EREMOVE.  This is useful in order to match the expectations
> of guests after reboot, and to match the behavior of real hardware.
> 
> The ioctl is a cleaner alternative to closing and reopening the
> /dev/sgx_vepc device; reopening /dev/sgx_vepc could be problematic in
> case userspace has sandboxed itself since the time it first opened the
> device, and has thus lost permissions to do so.
> 
> If possible, I would like these patches to be included in 5.15 through
> either the x86 or the KVM tree.
> 
> Thanks,
> 
> Paolo
> 
> Changes from RFC:
> - improved commit messages, added documentation
> - renamed ioctl from SGX_IOC_VEPC_REMOVE to SGX_IOC_VEPC_REMOVE_ALL
> 
> Change from v1:
> - fixed documentation and code to cover SGX_ENCLAVE_ACT errors
> - removed Tested-by since the code is quite different now
> 
> Changes from v2:
> - return EBUSY also if EREMOVE causes a general protection fault
> 
> Paolo Bonzini (2):
>   x86: sgx_vepc: extract sgx_vepc_remove_page
>   x86: sgx_vepc: implement SGX_IOC_VEPC_REMOVE_ALL ioctl
> 
>  Documentation/x86/sgx.rst       | 35 +++++++++++++++++++++
>  arch/x86/include/uapi/asm/sgx.h |  2 ++
>  arch/x86/kernel/cpu/sgx/virt.c  | 63 ++++++++++++++++++++++++++++++---
>  3 files changed, 95 insertions(+), 5 deletions(-)
> 

BTW, do you already have patch for QEMU somewhere, which uses
this new functionality? Would like to peek at it just to see
the usage pattern.

Also, can you provide some way to stress test QEMU so that this
code path gets executed? I'm already using QEMU as my main test
platform for SGX, so it's not a huge stretch for me to test it.

This way I can also provide tested-by for the corresponding QEMU
path.

/Jarkko

Re: [PATCH v3 0/2] x86: sgx_vepc: implement ioctl to EREMOVE all pages

[Borislav Petkov]

On Sat, Oct 16, 2021 at 03:14:32AM -0400, Paolo Bonzini wrote:
> Add to /dev/sgx_vepc a ioctl that brings vEPC pages back to uninitialized
> state with EREMOVE.  This is useful in order to match the expectations
> of guests after reboot, and to match the behavior of real hardware.
> 
> The ioctl is a cleaner alternative to closing and reopening the
> /dev/sgx_vepc device; reopening /dev/sgx_vepc could be problematic in
> case userspace has sandboxed itself since the time it first opened the
> device, and has thus lost permissions to do so.
> 
> If possible, I would like these patches to be included in 5.15 through
> either the x86 or the KVM tree.
> 
> Thanks,
> 
> Paolo
> 
> Changes from RFC:
> - improved commit messages, added documentation
> - renamed ioctl from SGX_IOC_VEPC_REMOVE to SGX_IOC_VEPC_REMOVE_ALL
> 
> Change from v1:
> - fixed documentation and code to cover SGX_ENCLAVE_ACT errors
> - removed Tested-by since the code is quite different now
> 
> Changes from v2:
> - return EBUSY also if EREMOVE causes a general protection fault
> 
> Paolo Bonzini (2):
>   x86: sgx_vepc: extract sgx_vepc_remove_page
>   x86: sgx_vepc: implement SGX_IOC_VEPC_REMOVE_ALL ioctl
> 
>  Documentation/x86/sgx.rst       | 35 +++++++++++++++++++++
>  arch/x86/include/uapi/asm/sgx.h |  2 ++
>  arch/x86/kernel/cpu/sgx/virt.c  | 63 ++++++++++++++++++++++++++++++---
>  3 files changed, 95 insertions(+), 5 deletions(-)

Sean,

are you happy with that version now?

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg

Re: [PATCH v3 0/2] x86: sgx_vepc: implement ioctl to EREMOVE all pages

[Paolo Bonzini]

On 18/10/21 14:51, Jarkko Sakkinen wrote:
> BTW, do you already have patch for QEMU somewhere, which uses
> this new functionality? Would like to peek at it just to see
> the usage pattern.

Yang was going to post them this week; the way to trigger them is just 
to do a reboot from within a Linux guest.

Paolo

> Also, can you provide some way to stress test QEMU so that this
> code path gets executed? I'm already using QEMU as my main test
> platform for SGX, so it's not a huge stretch for me to test it.
> 
> This way I can also provide tested-by for the corresponding QEMU
> path.

Re: [PATCH v3 0/2] x86: sgx_vepc: implement ioctl to EREMOVE all pages

[Jarkko Sakkinen]

On Mon, 2021-10-18 at 15:03 +0200, Paolo Bonzini wrote:
> On 18/10/21 14:51, Jarkko Sakkinen wrote:
> > BTW, do you already have patch for QEMU somewhere, which uses
> > this new functionality? Would like to peek at it just to see
> > the usage pattern.
> 
> Yang was going to post them this week; the way to trigger them is just 
> to do a reboot from within a Linux guest.
> 

Yang, please CC to me the full patch set, if possible. Thanks.

/Jarkko

Re: [PATCH v3 2/2] x86: sgx_vepc: implement SGX_IOC_VEPC_REMOVE ioctl

[Sean Christopherson]

> +static long sgx_vepc_remove_all(struct sgx_vepc *vepc)
> +{
> +	struct sgx_epc_page *entry;
> +	unsigned long index;
> +	long failures = 0;
> +
> +	xa_for_each(&vepc->page_array, index, entry) {
> +		int ret = sgx_vepc_remove_page(entry);
> +		if (ret) {
> +			if (ret == SGX_CHILD_PRESENT) {

There's a ton of documentation in the changelog and official docs, but a comment
here would also be helpful.

> +				failures++;
> +			} else {
> +				/*
> +				 * Unlike in sgx_vepc_free_page, userspace might
> +				 * call the ioctl while logical processors are
> +				 * running in the enclave, or cause faults due
> +				 * to concurrent access to pages under the same
> +				 * SECS.  So we cannot warn, we just report it.

Technically the kernel can WARN on #PF[*], as EREMOVE only hits #PF if there's a
legitimate #PF or if the target page is not an EPC page.  FWIW, the comments are
a little less compressed if the if statements aren't nested.

		if (ret == SGX_CHILD_PRESENT) {
			/*
			 * Track and return the number of SECS pages that cannot
			 * be removed because they have child EPC pages (in this
			 * vEPC or a different vEPC).
			 */
			failures++;
		} else if (ret) {
			/*
			 * Report errors due to #GP or SGX_ENCLAVE_ACT, but do
			 * not WARN as userspace can induce said failures by
			 * calling the ioctl concurrently on multiple vEPCs or
			 * while one or more CPUs is running the enclave.  Only
			 * a #PF on EREMOVE indicates a kernel/hardware issue.
			 */
			WARN_ON_ONCE(encls_faulted(ret) &&
				     ENCLS_TRAPNR(ret) == X86_TRAP_PF);
			return -EBUSY;
		}

[*] SGX1 hardware has an erratum where it signals #GP instead of #PF, but that's
    ok in this case because it's a false negative, not a false positive.

> +				 */
> +				return -EBUSY;
> +			}
> +		}
> +		cond_resched();
> +	}
> +
> +	/*
> +	 * Return the number of pages that failed to be removed, so
> +	 * userspace knows that there are still SECS pages lying
> +	 * around.

Nit, the comment doesn't need to span three lines.

	/*
	 * Return the number of pages that failed to be removed, so userspace
	 * knows that there are still SECS pages lying around.
	 */

> +	 */
> +	return failures;
> +}

Re: [PATCH v3 2/2] x86: sgx_vepc: implement SGX_IOC_VEPC_REMOVE ioctl

[Paolo Bonzini]

On 18/10/21 19:17, Sean Christopherson wrote:
> 			/*
> 			 * Report errors due to #GP or SGX_ENCLAVE_ACT, but do
> 			 * not WARN as userspace can induce said failures by
> 			 * calling the ioctl concurrently on multiple vEPCs or
> 			 * while one or more CPUs is running the enclave.  Only
> 			 * a #PF on EREMOVE indicates a kernel/hardware issue.
> 			 */
> 			WARN_ON_ONCE(encls_faulted(ret) &&
> 				     ENCLS_TRAPNR(ret) == X86_TRAP_PF);

or != X86_TRAP_GP, just to avoid having a v5? :)

Paolo

Re: [PATCH v3 2/2] x86: sgx_vepc: implement SGX_IOC_VEPC_REMOVE ioctl

[Sean Christopherson]

On Mon, Oct 18, 2021, Paolo Bonzini wrote:
> On 18/10/21 19:17, Sean Christopherson wrote:
> > 			/*
> > 			 * Report errors due to #GP or SGX_ENCLAVE_ACT, but do
> > 			 * not WARN as userspace can induce said failures by
> > 			 * calling the ioctl concurrently on multiple vEPCs or
> > 			 * while one or more CPUs is running the enclave.  Only
> > 			 * a #PF on EREMOVE indicates a kernel/hardware issue.
> > 			 */
> > 			WARN_ON_ONCE(encls_faulted(ret) &&
> > 				     ENCLS_TRAPNR(ret) == X86_TRAP_PF);
> 
> or != X86_TRAP_GP, just to avoid having a v5? :)

LOL, good point, that's indeed better.

Re: [PATCH v3 0/2] x86: sgx_vepc: implement ioctl to EREMOVE all pages

[Yang Zhong]

On Sat, Oct 16, 2021 at 03:14:32AM -0400, Paolo Bonzini wrote:
> Add to /dev/sgx_vepc a ioctl that brings vEPC pages back to uninitialized
> state with EREMOVE.  This is useful in order to match the expectations
> of guests after reboot, and to match the behavior of real hardware.
> 
> The ioctl is a cleaner alternative to closing and reopening the
> /dev/sgx_vepc device; reopening /dev/sgx_vepc could be problematic in
> case userspace has sandboxed itself since the time it first opened the
> device, and has thus lost permissions to do so.
> 
> If possible, I would like these patches to be included in 5.15 through
> either the x86 or the KVM tree.
>

  Paolo, i verified this version with Qemu reset patch, and passed all
  test.
  (1). sgx windows guest reset test.
  (2). single vepc or multiple vepc reset with 100 enlclaves are running
       in the guest.

  Thanks，

  Yang


 
> Thanks,
> 
> Paolo
> 
> Changes from RFC:
> - improved commit messages, added documentation
> - renamed ioctl from SGX_IOC_VEPC_REMOVE to SGX_IOC_VEPC_REMOVE_ALL
> 
> Change from v1:
> - fixed documentation and code to cover SGX_ENCLAVE_ACT errors
> - removed Tested-by since the code is quite different now
> 
> Changes from v2:
> - return EBUSY also if EREMOVE causes a general protection fault
> 
> Paolo Bonzini (2):
>   x86: sgx_vepc: extract sgx_vepc_remove_page
>   x86: sgx_vepc: implement SGX_IOC_VEPC_REMOVE_ALL ioctl
> 
>  Documentation/x86/sgx.rst       | 35 +++++++++++++++++++++
>  arch/x86/include/uapi/asm/sgx.h |  2 ++
>  arch/x86/kernel/cpu/sgx/virt.c  | 63 ++++++++++++++++++++++++++++++---
>  3 files changed, 95 insertions(+), 5 deletions(-)
> 
> -- 
> 2.27.0