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

[PATCH 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

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       | 14 ++++++++++
 arch/x86/include/uapi/asm/sgx.h |  2 ++
 arch/x86/kernel/cpu/sgx/virt.c  | 48 ++++++++++++++++++++++++++++++---
 3 files changed, 61 insertions(+), 3 deletions(-)

-- 
2.27.0

[PATCH 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, or running in a mount namespace that
does not have access to /dev; both are doable with pre-opened file
descriptors and/or 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 pulling the EREMOVE into a separate
function.

Tested-by: Yang Zhong <yang.zhong@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kernel/cpu/sgx/virt.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
index 64511c4a5200..59b9c13121cd 100644
--- a/arch/x86/kernel/cpu/sgx/virt.c
+++ b/arch/x86/kernel/cpu/sgx/virt.c
@@ -111,7 +111,7 @@ 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;
 
@@ -140,11 +140,17 @@ static int sgx_vepc_free_page(struct sgx_epc_page *epc_page)
 		 */
 		WARN_ONCE(ret != SGX_CHILD_PRESENT, EREMOVE_ERROR_MESSAGE,
 			  ret, ret);
-		return ret;
 	}
+	return ret;
+}
 
-	sgx_free_epc_page(epc_page);
+static int sgx_vepc_free_page(struct sgx_epc_page *epc_page)
+{
+	int ret = sgx_vepc_remove_page(epc_page);
+	if (ret)
+		return ret;
 
+	sgx_free_epc_page(epc_page);
 	return 0;
 }
 
-- 
2.27.0

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

[Jarkko Sakkinen]

On Mon, 2021-09-20 at 08:54 -0400, Paolo Bonzini wrote:
> 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.

I would consider replacing

"For bare-metal SGX on real hardware, the hardware provides guarantees
SGX state at reboot.  For instance, all pages start out uninitialized."

something like

"On bare-metal SGX, start of a power cycle zeros all of its reserved
memory. This happens after every reboot, but in addition to that
happens after waking up from any of the sleep states."

I can speculate and imagine where this might useful, but no matter how
trivial or complex it is, this patch needs to nail a concrete usage
example. I'd presume you know well the exact changes needed for QEMU, so
from that knowledge it should be easy to write the motivational part.

For instance, point out where it is needed in QEMU and why. I.e. why
you end up in the first place having to re-use vepc buffers (or whatever
they should be called) in QEMU. When that is taken care of, then there is
a red line to eventually ack these patches.

About the motivation.

In Linux we do have a mechanism to take care of this in a guest, for which
motivation was actually first and foremost kexec. It was not done to let VMM to
give a corrupted memory state to a guest.

Even to a Linux guest, since EPC should stil be represented in the state that
matches the hardware.  It'd be essentially a corrupted state, even if there was
measures to resist this. Windows guests failing is essentially a side-effect
of an issue, not an issue in the Windows guests.

Since QEMU needs to reinitialize VEPC buffers for guests, it should be as
efficient as we ever can make it. Just fill the gap of understanding why
QEMU needs to do this for guest. This is exactly kind of stuff that you want
have documented in the commit log for future :-)

/Jarkko

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

[Jarkko Sakkinen]

On Tue, 2021-09-21 at 22:44 +0300, Jarkko Sakkinen wrote:
> Even to a Linux guest, since EPC should stil be represented in the state that
> matches the hardware.  It'd be essentially a corrupted state, even if there was
> measures to resist this. Windows guests failing is essentially a side-effect
> of an issue, not an issue in the Windows guests.

Ugh, typos, sorry. Even to a Linux guest it would be illegit what I was meaning
to say...

/Jarkko

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

[Paolo Bonzini]

On 21/09/21 21:44, Jarkko Sakkinen wrote:
> "On bare-metal SGX, start of a power cycle zeros all of its reserved 
> memory. This happens after every reboot, but in addition to that 
> happens after waking up from any of the sleep states."
> 
> I can speculate and imagine where this might useful, but no matter
> how trivial or complex it is, this patch needs to nail a concrete
> usage example. I'd presume you know well the exact changes needed for
> QEMU, so from that knowledge it should be easy to write the
> motivational part.

Assuming that it's obvious that QEMU knows how to reset a machine (which 
includes writes to the ACPI reset register, or wakeup from sleep 
states), the question of "why does userspace reuse vEPC" should be 
answered by this paragraph:

"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, or running in a mount namespace that
does not have access to /dev; both are doable with pre-opened file
descriptors and/or SCM_RIGHTS file descriptor passing)."

> Even to a Linux guest, since EPC should stil be represented in the
> state that matches the hardware.  It'd be essentially a corrupted
> state, even if there was measures to resist this. Windows guests
> failing is essentially a side-effect of an issue, not an issue in the
> Windows guests.

Right, Linux is more liberal than it needs to be and ksgxd does the 
EREMOVE itself at the beginning (__sgx_sanitize_pages).  Windows has 
stronger expectations of what can and cannot happen before it boots, 
which are entirely justified.

Paolo

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

[Jarkko Sakkinen]

On Thu, 2021-09-23 at 14:08 +0200, Paolo Bonzini wrote:
> On 21/09/21 21:44, Jarkko Sakkinen wrote:
> > "On bare-metal SGX, start of a power cycle zeros all of its reserved 
> > memory. This happens after every reboot, but in addition to that 
> > happens after waking up from any of the sleep states."
> > 
> > I can speculate and imagine where this might useful, but no matter
> > how trivial or complex it is, this patch needs to nail a concrete
> > usage example. I'd presume you know well the exact changes needed for
> > QEMU, so from that knowledge it should be easy to write the
> > motivational part.
> 
> Assuming that it's obvious that QEMU knows how to reset a machine (which 
> includes writes to the ACPI reset register, or wakeup from sleep 
> states), the question of "why does userspace reuse vEPC" should be 
> answered by this paragraph:
> 
> "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, or running in a mount namespace that
> does not have access to /dev; both are doable with pre-opened file
> descriptors and/or SCM_RIGHTS file descriptor passing)."

Right, this makes sense.

> 
> > Even to a Linux guest, since EPC should stil be represented in the
> > state that matches the hardware.  It'd be essentially a corrupted
> > state, even if there was measures to resist this. Windows guests
> > failing is essentially a side-effect of an issue, not an issue in the
> > Windows guests.
> 
> Right, Linux is more liberal than it needs to be and ksgxd does the 
> EREMOVE itself at the beginning (__sgx_sanitize_pages).  Windows has 
> stronger expectations of what can and cannot happen before it boots, 
> which are entirely justified.
> 
> Paolo

Yep. We do it for kexec(). Alternative would be to zero at the time
of kexec() but this way things are just way more simpler, e.g. the
whole behaviour is local to the driver...

/Jarkko

[PATCH 2/2] x86: sgx_vepc: implement SGX_IOC_VEPC_REMOVE_ALL 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 be sandboxing themselves before the
guest starts in order to mitigate exploits from untrusted guests,
forbidding further calls to open().

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, so the ioctl returns the number of EREMOVE failures.  The
correct usage is documented in sgx.rst.

Tested-by: Yang Zhong <yang.zhong@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 Documentation/x86/sgx.rst       | 14 +++++++++++++
 arch/x86/include/uapi/asm/sgx.h |  2 ++
 arch/x86/kernel/cpu/sgx/virt.c  | 36 +++++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+)

diff --git a/Documentation/x86/sgx.rst b/Documentation/x86/sgx.rst
index dd0ac96ff9ef..b855db96c6c6 100644
--- a/Documentation/x86/sgx.rst
+++ b/Documentation/x86/sgx.rst
@@ -250,3 +250,17 @@ 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.
+
+Some guests, such as Windows, require that all pages are uninitialized
+at startup or 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.
+
+The ioctl will often not able to remove SECS pages, in case their child
+pages have not gone through ``EREMOVE`` yet; therefore, the ioctl returns the
+number of pages that failed to be removed.  ``SGX_IOC_VEPC_REMOVE_ALL`` should
+first be invoked on all the ``/dev/sgx_vepc`` file descriptors mapped
+into the guest; a second call to the ioctl will be able to remove all
+leftover pages and will return 0.  Any other return value on the second call
+would be a symptom of a bug in either Linux or the userspace client.
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 9690d6899ad9..f79d84ce8033 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 59b9c13121cd..81dc186fda2e 100644
--- a/arch/x86/kernel/cpu/sgx/virt.c
+++ b/arch/x86/kernel/cpu/sgx/virt.c
@@ -154,6 +154,24 @@ 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)
+		if (sgx_vepc_remove_page(entry))
+			failures++;
+
+	/*
+	 * 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;
@@ -239,9 +257,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 2/2] x86: sgx_vepc: implement SGX_IOC_VEPC_REMOVE_ALL ioctl

[Kai Huang]

On Mon, 20 Sep 2021 08:54:01 -0400 Paolo Bonzini wrote:
> 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 be sandboxing themselves before the
> guest starts in order to mitigate exploits from untrusted guests,
> forbidding further calls to open().
> 
> 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, so the ioctl returns the number of EREMOVE failures.  The
> correct usage is documented in sgx.rst.
> 
> Tested-by: Yang Zhong <yang.zhong@intel.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  Documentation/x86/sgx.rst       | 14 +++++++++++++
>  arch/x86/include/uapi/asm/sgx.h |  2 ++
>  arch/x86/kernel/cpu/sgx/virt.c  | 36 +++++++++++++++++++++++++++++++++
>  3 files changed, 52 insertions(+)
> 
> diff --git a/Documentation/x86/sgx.rst b/Documentation/x86/sgx.rst
> index dd0ac96ff9ef..b855db96c6c6 100644
> --- a/Documentation/x86/sgx.rst
> +++ b/Documentation/x86/sgx.rst
> @@ -250,3 +250,17 @@ 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.
> +
> +Some guests, such as Windows, require that all pages are uninitialized
> +at startup or after a guest reboot.  

I would say this is not requirement of some windows guests, but requirement of
correctly emulating architectural behaviour.  On bare-metal EPC is lost on
reboot, so on VM reboot, virtual EPC should be reset too, regardless what
guest VM is.  

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.
> +
> +The ioctl will often not able to remove SECS pages, in case their child
> +pages have not gone through ``EREMOVE`` yet; therefore, the ioctl returns the
> +number of pages that failed to be removed.  ``SGX_IOC_VEPC_REMOVE_ALL`` should
> +first be invoked on all the ``/dev/sgx_vepc`` file descriptors mapped
> +into the guest; a second call to the ioctl will be able to remove all
> +leftover pages and will return 0.  Any other return value on the second call
> +would be a symptom of a bug in either Linux or the userspace client.

Maybe also worth to mention userspace should guarantee there's no vcpu running
inside guest enclave when resetting guest's virtual EPC.

> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index 9690d6899ad9..f79d84ce8033 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 59b9c13121cd..81dc186fda2e 100644
> --- a/arch/x86/kernel/cpu/sgx/virt.c
> +++ b/arch/x86/kernel/cpu/sgx/virt.c
> @@ -154,6 +154,24 @@ 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)
> +		if (sgx_vepc_remove_page(entry))
> +			failures++;

I think need to hold vepc->lock?

> +
> +	/*
> +	 * 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;
> @@ -239,9 +257,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 2/2] x86: sgx_vepc: implement SGX_IOC_VEPC_REMOVE_ALL ioctl

[Dave Hansen]

On 9/20/21 3:17 PM, Kai Huang wrote:
>> +The ioctl will often not able to remove SECS pages, in case their child
>> +pages have not gone through ``EREMOVE`` yet; therefore, the ioctl returns the
>> +number of pages that failed to be removed.  ``SGX_IOC_VEPC_REMOVE_ALL`` should
>> +first be invoked on all the ``/dev/sgx_vepc`` file descriptors mapped
>> +into the guest; a second call to the ioctl will be able to remove all
>> +leftover pages and will return 0.  Any other return value on the second call
>> +would be a symptom of a bug in either Linux or the userspace client.
> Maybe also worth to mention userspace should guarantee there's no vcpu running
> inside guest enclave when resetting guest's virtual EPC.

Why, specifically?

Is it because EREMOVE will also fail if there is a CPU running in the
enclave?

Maybe we should say something like:

The ioctl() returns the number of pages where removal failed.  Callers
of the ioctl() need to handle two sources of failure:

1) Page removal will always fail when any thread is running in the
   enclave to which the page belongs.  ioctl() users can avoid these
   failures by preventing execution of any vcpu which maps the virtual
   EPC.

2) Page removal will also fail for SGX "SECS" metadata pages which still
   have child pages.  SECS pages can be removed by executing
   ``SGX_IOC_VEPC_REMOVE_ALL`` on all ``/dev/sgx_vepc`` file descriptors
   mapped into the guest.  Yhis 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.

   It indicates a bug in the kernel or the userspace client if any of
   the second round of ``SGX_IOC_VEPC_REMOVE_ALL`` calls fails (has a
   return code other than 0).

Re: [PATCH 2/2] x86: sgx_vepc: implement SGX_IOC_VEPC_REMOVE_ALL ioctl

[Paolo Bonzini]

On 21/09/21 01:09, Dave Hansen wrote:
>> Maybe also worth to mention userspace should guarantee there's no vcpu running
>> inside guest enclave when resetting guest's virtual EPC.
> Why, specifically?
> 
> Is it because EREMOVE will also fail if there is a CPU running in the
> enclave?

Yes, and SGX_ENCLAVE_ACT would cause a WARN.  Good catch, Kai, I'll fix it.

Paolo