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

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

[Paolo Bonzini]

Based on discussions from the previous week(end), this series implements
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.

The patches are untested, but I am posting them because they are simple
and so that Yang Zhong can try using them in QEMU.

Paolo

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

 arch/x86/include/uapi/asm/sgx.h |  2 ++
 arch/x86/kernel/cpu/sgx/virt.c  | 48 ++++++++++++++++++++++++++++++---
 2 files changed, 47 insertions(+), 3 deletions(-)

-- 
2.27.0

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

[Paolo Bonzini]

Windows expects all pages to be in uninitialized state on startup.
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.

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

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

[Paolo Bonzini]

Windows expects all pages to be in uninitialized state on startup.
Add a ioctl that does this with EREMOVE, so that userspace can bring
the pages back to this state also when resetting the VM.
Pure userspace implementations, such as closing and reopening the device,
are racy.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/uapi/asm/sgx.h |  2 ++
 arch/x86/kernel/cpu/sgx/virt.c  | 36 +++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

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 \
+	_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:
+		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 1/2] x86: sgx_vepc: extract sgx_vepc_remove_page

[Dave Hansen]

On 9/13/21 6:11 AM, Paolo Bonzini wrote:
> Windows expects all pages to be in uninitialized state on startup.
> 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.

Hi Paolo,

How does this end up happening in the first place?

All enclave pages should start out on 'sgx_dirty_page_list' and
ksgxd sanitizes them with EREMOVE before making them available.  That
should cover EREMOVE after reboots while SGX pages are initialized,
including kexec().

sgx_vepc_free_page() should do the same for pages that a guest not not
clean up properly.

sgx_encl_free_epc_page() does an EREMOVE after a normal enclave has used
a page.

Those are the only three cases that I can think of.  So, it sounds like
one of those is buggy, or there's another unexpected path out there.
Ultimately, I think it would be really handy if we could do this EREMOVE
implicitly and without any new ABI.

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

[Paolo Bonzini]

On 13/09/21 16:05, Dave Hansen wrote:
> On 9/13/21 6:11 AM, Paolo Bonzini wrote:
>> Windows expects all pages to be in uninitialized state on startup.
>> 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.
> 
> Hi Paolo,
> 
> How does this end up happening in the first place?
> 
> All enclave pages should start out on 'sgx_dirty_page_list' and
> ksgxd sanitizes them with EREMOVE before making them available.  That
> should cover EREMOVE after reboots while SGX pages are initialized,
> including kexec().

By "Windows startup" I mean even after guest reboot.  Because another 
process could sneak in and steal your EPC pages between a close() and an 
open(), I'd like to have a way to EREMOVE the pages while keeping them 
assigned to the specific vEPC instance, i.e. *without* going through 
sgx_vepc_free_page().

Thanks,

Paolo

> sgx_vepc_free_page() should do the same for pages that a guest not not
> clean up properly.
> 
> sgx_encl_free_epc_page() does an EREMOVE after a normal enclave has used
> a page.
> 
> Those are the only three cases that I can think of.  So, it sounds like
> one of those is buggy, or there's another unexpected path out there.
> Ultimately, I think it would be really handy if we could do this EREMOVE
> implicitly and without any new ABI.
> 

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

[Dave Hansen]

On 9/13/21 7:24 AM, Paolo Bonzini wrote:
>> How does this end up happening in the first place?
>>
>> All enclave pages should start out on 'sgx_dirty_page_list' and
>> ksgxd sanitizes them with EREMOVE before making them available.  That
>> should cover EREMOVE after reboots while SGX pages are initialized,
>> including kexec().
> 
> By "Windows startup" I mean even after guest reboot.  Because another
> process could sneak in and steal your EPC pages between a close() and an
> open(), I'd like to have a way to EREMOVE the pages while keeping them
> assigned to the specific vEPC instance, i.e. *without* going through
> sgx_vepc_free_page().

Oh, so you want fresh EPC state for the guest, but you're concerned that
the previous guest might have left them in a bad state.  The current
method of getting a new vepc instance (which guarantees fresh state) has
some other downsides.

Can't another process steal pages via sgxd and reclaim at any time?
What's the extra concern here about going through a close()/open()
cycle?  Performance?

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

[Paolo Bonzini]

On 13/09/21 16:55, Dave Hansen wrote:
>> By "Windows startup" I mean even after guest reboot.  Because another
>> process could sneak in and steal your EPC pages between a close() and an
>> open(), I'd like to have a way to EREMOVE the pages while keeping them
>> assigned to the specific vEPC instance, i.e.*without*  going through
>> sgx_vepc_free_page().
> Oh, so you want fresh EPC state for the guest, but you're concerned that
> the previous guest might have left them in a bad state.  The current
> method of getting a new vepc instance (which guarantees fresh state) has
> some other downsides.
> 
> Can't another process steal pages via sgxd and reclaim at any time?

vEPC pages never call sgx_mark_page_reclaimable, don't they?

> What's the extra concern here about going through a close()/open()
> cycle?  Performance?

Apart from reclaiming, /dev/sgx_vepc might disappear between the first 
open() and subsequent ones.

Paolo

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

[Dave Hansen]

On 9/13/21 8:14 AM, Paolo Bonzini wrote:
> On 13/09/21 16:55, Dave Hansen wrote:
>>> By "Windows startup" I mean even after guest reboot.  Because another
>>> process could sneak in and steal your EPC pages between a close() and an
>>> open(), I'd like to have a way to EREMOVE the pages while keeping them
>>> assigned to the specific vEPC instance, i.e.*without*  going through
>>> sgx_vepc_free_page().
>> Oh, so you want fresh EPC state for the guest, but you're concerned that
>> the previous guest might have left them in a bad state.  The current
>> method of getting a new vepc instance (which guarantees fresh state) has
>> some other downsides.
>>
>> Can't another process steal pages via sgxd and reclaim at any time?
> 
> vEPC pages never call sgx_mark_page_reclaimable, don't they?

Oh, I was just looking that they were on the SGX LRU.  You might be right.

But, we certainly don't want the fact that they are unreclaimable today
to be part of the ABI.  It's more of a bug than a feature.

>> What's the extra concern here about going through a close()/open()
>> cycle?  Performance?
> 
> Apart from reclaiming, /dev/sgx_vepc might disappear between the first
> open() and subsequent ones.

Aside from it being rm'd, I don't think that's possible.

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

[Paolo Bonzini]

On 13/09/21 17:29, Dave Hansen wrote:
> On 9/13/21 8:14 AM, Paolo Bonzini wrote:
>> On 13/09/21 16:55, Dave Hansen wrote:
>>>> By "Windows startup" I mean even after guest reboot.  Because another
>>>> process could sneak in and steal your EPC pages between a close() and an
>>>> open(), I'd like to have a way to EREMOVE the pages while keeping them
>>>> assigned to the specific vEPC instance, i.e.*without*  going through
>>>> sgx_vepc_free_page().
>>> Oh, so you want fresh EPC state for the guest, but you're concerned that
>>> the previous guest might have left them in a bad state.  The current
>>> method of getting a new vepc instance (which guarantees fresh state) has
>>> some other downsides.
>>>
>>> Can't another process steal pages via sgxd and reclaim at any time?
>>
>> vEPC pages never call sgx_mark_page_reclaimable, don't they?
> 
> Oh, I was just looking that they were on the SGX LRU.  You might be right.
> But, we certainly don't want the fact that they are unreclaimable today
> to be part of the ABI.  It's more of a bug than a feature.

Sure, that's fine.

>>> What's the extra concern here about going through a close()/open()
>>> cycle?  Performance?
>>
>> Apart from reclaiming, /dev/sgx_vepc might disappear between the first
>> open() and subsequent ones.
> 
> Aside from it being rm'd, I don't think that's possible.
> 

Being rm'd would be a possibility in principle, and it would be ugly for 
it to cause issues on running VMs.  Also I'd like for it to be able to 
pass /dev/sgx_vepc in via a file descriptor, and run QEMU in a chroot or 
a mount namespace.  Alternatively, with seccomp it may be possible to 
sandbox a running QEMU process in such a way that open() is forbidden at 
runtime (all hotplug is done via file descriptor passing); it is not yet 
possible, but it is a goal.

Paolo

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

[Dave Hansen]

On 9/13/21 11:35 AM, Paolo Bonzini wrote:
>>> Apart from reclaiming, /dev/sgx_vepc might disappear between the first
>>> open() and subsequent ones.
>>
>> Aside from it being rm'd, I don't think that's possible.
>>
> 
> Being rm'd would be a possibility in principle, and it would be ugly for
> it to cause issues on running VMs.  Also I'd like for it to be able to
> pass /dev/sgx_vepc in via a file descriptor, and run QEMU in a chroot or
> a mount namespace.  Alternatively, with seccomp it may be possible to
> sandbox a running QEMU process in such a way that open() is forbidden at
> runtime (all hotplug is done via file descriptor passing); it is not yet
> possible, but it is a goal.

OK, so maybe another way of saying this:

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, vepc users have a problem: they might want to run an OS that
expects to be booted with clean, fully uninitialized SGX state, just as
it would be on bare-metal.  Right now, the only way to get that is to
create a new vepc instance.  That might not be possible in all
configurations, like if the permission to open(/dev/sgx_vepc) has been
lost since the VM was first booted.

Windows has these expectations about booting with fully uninitialized
state.  There are also a number of environments where QEMU is sandboxed
or drops permissions in a way that prevent free and open access to
/dev/sgx_vepc.

So good so far?

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

[Dave Hansen]

On 9/13/21 6:11 AM, Paolo Bonzini wrote:
> +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;
> +}

I'm not sure the retry logic should be in userspace.  Also, is this
strictly limited to SECS pages?  It could also happen if there were
enclaves running that used the page.  Granted, userspace can probably
stop all the vcpus, but the *interface* doesn't prevent it being called
like that.

What else can userspace do but:

	ret = ioctl(fd, SGX_IOC_VEPC_REMOVE);
	if (ret)
		ret = ioctl(fd, SGX_IOC_VEPC_REMOVE);
	if (ret)
		printf("uh oh\n");

We already have existing code to gather up the pages that couldn't be
EREMOVE'd and selectively EREMOVE them again.  Why not reuse that code
here?  If there is 100GB of EPC, it's gotta be painful to run through
the ->page_array twice when once plus a small list iteration will do.

Which reminds me...  Do we need a cond_resched() in there or anything?
That loop could theoretically get really, really long.

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

[Jarkko Sakkinen]

On Mon, 2021-09-13 at 09:11 -0400, Paolo Bonzini wrote:
> Windows expects all pages to be in uninitialized state on startup.
> 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.

So what makes it racy, and what do mean by racy in this case?

I.e. you have to do open() and mmap(), and munmap() + close()
for removal. Depending on situation that is racy or not...

And is "Windows" more precisely a "Windows guest running in
Linux QEMU host"? It's ambiguous..

/Jarkko

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

[Jarkko Sakkinen]

On Mon, 2021-09-13 at 16:24 +0200, Paolo Bonzini wrote:
> On 13/09/21 16:05, Dave Hansen wrote:
> > On 9/13/21 6:11 AM, Paolo Bonzini wrote:
> > > Windows expects all pages to be in uninitialized state on startup.
> > > 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.
> > 
> > Hi Paolo,
> > 
> > How does this end up happening in the first place?
> > 
> > All enclave pages should start out on 'sgx_dirty_page_list' and
> > ksgxd sanitizes them with EREMOVE before making them available.  That
> > should cover EREMOVE after reboots while SGX pages are initialized,
> > including kexec().
> 
> By "Windows startup" I mean even after guest reboot.  Because another 
> process could sneak in and steal your EPC pages between a close() and an 
> open(), I'd like to have a way to EREMOVE the pages while keeping them 
> assigned to the specific vEPC instance, i.e. *without* going through 
> sgx_vepc_free_page().

Isn't "other process in and steal your EPC pages" more like sysadmin
problem, rather than software?

I'm lacking of understanding what would be the collateral damage in
the end.

/Jarkko

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

[Sean Christopherson]

On Mon, Sep 13, 2021, Dave Hansen wrote:
> On 9/13/21 6:11 AM, Paolo Bonzini wrote:
> > +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;
> > +}
> 
> I'm not sure the retry logic should be in userspace.  Also, is this
> strictly limited to SECS pages?  It could also happen if there were
> enclaves running that used the page.  Granted, userspace can probably
> stop all the vcpus, but the *interface* doesn't prevent it being called
> like that.

The general rule for KVM is that so long as userspace abuse of running vCPUs (or
other concurrent operations) doesn't put the kernel/platform at risk, it's
userspace's responsibility to not screw up.  The main argument being that there
are myriad ways the VMM can DoS the guest without having to abuse an ioctl().

> What else can userspace do but:
> 
> 	ret = ioctl(fd, SGX_IOC_VEPC_REMOVE);
> 	if (ret)
> 		ret = ioctl(fd, SGX_IOC_VEPC_REMOVE);
> 	if (ret)
> 		printf("uh oh\n");
> 
> We already have existing code to gather up the pages that couldn't be
> EREMOVE'd and selectively EREMOVE them again.  Why not reuse that code
> here?  If there is 100GB of EPC, it's gotta be painful to run through
> the ->page_array twice when once plus a small list iteration will do.

My argument against handling this fully in the kernel is that to handle a vNUMA
setup with multiple vEPC sections, the ioctl() would need to a take a set of file
descriptors to handle the case where an SECS is pinned by a child page in a
diferent vEPC.  It would also require an extra list_head per page (or dynamic
allocations), as the list_head in sgx_epc_page will (likely, eventually) be needed
to handle EPC OOM.  In the teardown case, sgx_epc_page.list can be used because
the pages are taken off the active/allocated list as part of teardown.

Neither issue is the end of the world, but IMO it's not worth burning the memory
and taking on extra complexity in the kernel for a relatively rare operation that's
slow as dirt anyways.

> Which reminds me...  Do we need a cond_resched() in there or anything?
> That loop could theoretically get really, really long.

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

[Jarkko Sakkinen]

On Mon, 2021-09-13 at 07:55 -0700, Dave Hansen wrote:
> On 9/13/21 7:24 AM, Paolo Bonzini wrote:
> > > How does this end up happening in the first place?
> > > 
> > > All enclave pages should start out on 'sgx_dirty_page_list' and
> > > ksgxd sanitizes them with EREMOVE before making them available.  That
> > > should cover EREMOVE after reboots while SGX pages are initialized,
> > > including kexec().
> > 
> > By "Windows startup" I mean even after guest reboot.  Because another
> > process could sneak in and steal your EPC pages between a close() and an
> > open(), I'd like to have a way to EREMOVE the pages while keeping them
> > assigned to the specific vEPC instance, i.e. *without* going through
> > sgx_vepc_free_page().
> 
> Oh, so you want fresh EPC state for the guest, but you're concerned that
> the previous guest might have left them in a bad state.  The current
> method of getting a new vepc instance (which guarantees fresh state) has
> some other downsides.
> 
> Can't another process steal pages via sgxd and reclaim at any time?
> What's the extra concern here about going through a close()/open()
> cycle?  Performance?

sgxd does not steal anything from vepc regions.

They are not part of the reclaiming process.

/Jarkko

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

[Jarkko Sakkinen]

On Mon, 2021-09-13 at 17:14 +0200, Paolo Bonzini wrote:
> On 13/09/21 16:55, Dave Hansen wrote:
> > > By "Windows startup" I mean even after guest reboot.  Because another
> > > process could sneak in and steal your EPC pages between a close() and an
> > > open(), I'd like to have a way to EREMOVE the pages while keeping them
> > > assigned to the specific vEPC instance, i.e.*without*  going through
> > > sgx_vepc_free_page().
> > Oh, so you want fresh EPC state for the guest, but you're concerned that
> > the previous guest might have left them in a bad state.  The current
> > method of getting a new vepc instance (which guarantees fresh state) has
> > some other downsides.
> > 
> > Can't another process steal pages via sgxd and reclaim at any time?
> 
> vEPC pages never call sgx_mark_page_reclaimable, don't they?
> 
> > What's the extra concern here about going through a close()/open()
> > cycle?  Performance?
> 
> Apart from reclaiming, /dev/sgx_vepc might disappear between the first 
> open() and subsequent ones.

If /dev/sgx_vepc dissapears, why is it a problem *for the software*, and
not a sysadmin problem?

I think that this is what the whole patch is lacking, why are we talking
about a software problem...

/Jarkko

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

[Jarkko Sakkinen]

On Mon, 2021-09-13 at 20:35 +0200, Paolo Bonzini wrote:
> On 13/09/21 17:29, Dave Hansen wrote:
> > On 9/13/21 8:14 AM, Paolo Bonzini wrote:
> > > On 13/09/21 16:55, Dave Hansen wrote:
> > > > > By "Windows startup" I mean even after guest reboot.  Because another
> > > > > process could sneak in and steal your EPC pages between a close() and an
> > > > > open(), I'd like to have a way to EREMOVE the pages while keeping them
> > > > > assigned to the specific vEPC instance, i.e.*without*  going through
> > > > > sgx_vepc_free_page().
> > > > Oh, so you want fresh EPC state for the guest, but you're concerned that
> > > > the previous guest might have left them in a bad state.  The current
> > > > method of getting a new vepc instance (which guarantees fresh state) has
> > > > some other downsides.
> > > > 
> > > > Can't another process steal pages via sgxd and reclaim at any time?
> > > 
> > > vEPC pages never call sgx_mark_page_reclaimable, don't they?
> > 
> > Oh, I was just looking that they were on the SGX LRU.  You might be right.
> > But, we certainly don't want the fact that they are unreclaimable today
> > to be part of the ABI.  It's more of a bug than a feature.
> 
> Sure, that's fine.
> 
> > > > What's the extra concern here about going through a close()/open()
> > > > cycle?  Performance?
> > > 
> > > Apart from reclaiming, /dev/sgx_vepc might disappear between the first
> > > open() and subsequent ones.
> > 
> > Aside from it being rm'd, I don't think that's possible.
> > 
> 
> Being rm'd would be a possibility in principle, and it would be ugly for 
> it to cause issues on running VMs.  Also I'd like for it to be able to 
> pass /dev/sgx_vepc in via a file descriptor, and run QEMU in a chroot or 
> a mount namespace.  Alternatively, with seccomp it may be possible to 
> sandbox a running QEMU process in such a way that open() is forbidden at 
> runtime (all hotplug is done via file descriptor passing); it is not yet 
> possible, but it is a goal.

AFAIK, as long you have open files for a device, they work
as expected.

/Jarkko

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

[Jarkko Sakkinen]

On Mon, 2021-09-13 at 12:25 -0700, Dave Hansen wrote:
> On 9/13/21 11:35 AM, Paolo Bonzini wrote:
> > > > Apart from reclaiming, /dev/sgx_vepc might disappear between the first
> > > > open() and subsequent ones.
> > > 
> > > Aside from it being rm'd, I don't think that's possible.
> > > 
> > 
> > Being rm'd would be a possibility in principle, and it would be ugly for
> > it to cause issues on running VMs.  Also I'd like for it to be able to
> > pass /dev/sgx_vepc in via a file descriptor, and run QEMU in a chroot or
> > a mount namespace.  Alternatively, with seccomp it may be possible to
> > sandbox a running QEMU process in such a way that open() is forbidden at
> > runtime (all hotplug is done via file descriptor passing); it is not yet
> > possible, but it is a goal.
> 
> OK, so maybe another way of saying this:
> 
> 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, vepc users have a problem: they might want to run an OS that
> expects to be booted with clean, fully uninitialized SGX state, just as
> it would be on bare-metal.  Right now, the only way to get that is to
> create a new vepc instance.  That might not be possible in all
> configurations, like if the permission to open(/dev/sgx_vepc) has been
> lost since the VM was first booted.

So you maintain your systems in a way that this does not happen?

/Jarkko

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

[Dave Hansen]

On 9/13/21 2:11 PM, Sean Christopherson wrote:
> My argument against handling this fully in the kernel is that to handle a vNUMA
> setup with multiple vEPC sections, the ioctl() would need to a take a set of file
> descriptors to handle the case where an SECS is pinned by a child page in a
> diferent vEPC.

Bah, I'm always forgetting about the multiple vepc fd's case.

I completely agree that there's no sane way to do this with a per-vepc
ioctl() when the EREMOVE failures can originate from other vepc instances.

The only other possible thing would be keep an mm_list for vepc
instances and have this ioctl() (or another interface) blast them all.
But that's going to be a heck of a lot more complicated than this is.

OK... you two are wearing me down on this one.

Let's just get this all documented in the changelogs, especially the
retry behavior.

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

[Paolo Bonzini]

On 13/09/21 23:13, Jarkko Sakkinen wrote:
>> Apart from reclaiming, /dev/sgx_vepc might disappear between the first
>> open() and subsequent ones.
>
> If /dev/sgx_vepc disappears, why is it a problem *for the software*, and
> not a sysadmin problem?

Rather than disappearing, it could be that a program first gets all the 
resources it needs before it gets malicious input, and then enter a 
restrictive sandbox.  In this case open() could be completely forbidden.

I will improve the documentation and changelogs when I post the non-RFC 
version; that could have been done better, sorry.

Paolo

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

[Yang Zhong]

On Mon, Sep 13, 2021 at 09:11:51AM -0400, Paolo Bonzini wrote:
> Based on discussions from the previous week(end), this series implements
> 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.
> 
> The patches are untested, but I am posting them because they are simple
> and so that Yang Zhong can try using them in QEMU.
> 

  Paolo, i re-implemented one reset patch in the Qemu side to call this ioctl(),
  and did some tests on Windows and Linux guest, the Windows/Linux guest reboot 
  work well.

  So, it is time for me to send this reset patch to Qemu community? or wait for
  this kernel patchset merged? thanks! 
     
  Yang


> Paolo
> 
> Paolo Bonzini (2):
>   x86: sgx_vepc: extract sgx_vepc_remove_page
>   x86: sgx_vepc: implement SGX_IOC_VEPC_REMOVE ioctl
> 
>  arch/x86/include/uapi/asm/sgx.h |  2 ++
>  arch/x86/kernel/cpu/sgx/virt.c  | 48 ++++++++++++++++++++++++++++++---
>  2 files changed, 47 insertions(+), 3 deletions(-)
> 
> -- 
> 2.27.0

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

[Paolo Bonzini]

On 14/09/21 09:10, Yang Zhong wrote:
> On Mon, Sep 13, 2021 at 09:11:51AM -0400, Paolo Bonzini wrote:
>> Based on discussions from the previous week(end), this series implements
>> 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.
>>
>> The patches are untested, but I am posting them because they are simple
>> and so that Yang Zhong can try using them in QEMU.
>>
> 
>    Paolo, i re-implemented one reset patch in the Qemu side to call this ioctl(),
>    and did some tests on Windows and Linux guest, the Windows/Linux guest reboot
>    work well.
> 
>    So, it is time for me to send this reset patch to Qemu community? or wait for
>    this kernel patchset merged? thanks!

Let's wait for this patch to be accepted first.  I'll wait a little more 
for Jarkko and Dave to comment on this, and include your "Tested-by".

I will also add cond_resched() on the final submission.

Paolo

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

[Kai Huang]

On Mon, 13 Sep 2021 09:11:53 -0400 Paolo Bonzini wrote:
> Windows expects all pages to be in uninitialized state on startup.
> Add a ioctl that does this with EREMOVE, so that userspace can bring
> the pages back to this state also when resetting the VM.
> Pure userspace implementations, such as closing and reopening the device,
> are racy.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/include/uapi/asm/sgx.h |  2 ++
>  arch/x86/kernel/cpu/sgx/virt.c  | 36 +++++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+)
> 
> 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 \
> +	_IO(SGX_MAGIC, 0x04)

Perhaps SGX_IOC_VEPC_RESET is better than REMOVE, since this ioctl doesn't
actually remove any EPC page from virtual EPC device, but just reset to a clean
slate (by using EREMOVE).

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

[Jarkko Sakkinen]

On Tue, 2021-09-14 at 07:36 +0200, Paolo Bonzini wrote:
> On 13/09/21 23:13, Jarkko Sakkinen wrote:
> > > Apart from reclaiming, /dev/sgx_vepc might disappear between the first
> > > open() and subsequent ones.
> > 
> > If /dev/sgx_vepc disappears, why is it a problem *for the software*, and
> > not a sysadmin problem?
> 
> Rather than disappearing, it could be that a program first gets all the 
> resources it needs before it gets malicious input, and then enter a 
> restrictive sandbox.  In this case open() could be completely forbidden.
> 
> I will improve the documentation and changelogs when I post the non-RFC 
> version; that could have been done better, sorry.
> 

No worries, just trying to get bottom of the actual issue.

/Jarkko

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

[Jarkko Sakkinen]

On Tue, 2021-09-14 at 12:19 +0200, Paolo Bonzini wrote:
> On 14/09/21 09:10, Yang Zhong wrote:
> > On Mon, Sep 13, 2021 at 09:11:51AM -0400, Paolo Bonzini wrote:
> > > Based on discussions from the previous week(end), this series implements
> > > 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.
> > > 
> > > The patches are untested, but I am posting them because they are simple
> > > and so that Yang Zhong can try using them in QEMU.
> > > 
> > 
> >    Paolo, i re-implemented one reset patch in the Qemu side to call this ioctl(),
> >    and did some tests on Windows and Linux guest, the Windows/Linux guest reboot
> >    work well.
> > 
> >    So, it is time for me to send this reset patch to Qemu community? or wait for
> >    this kernel patchset merged? thanks!
> 
> Let's wait for this patch to be accepted first.  I'll wait a little more 
> for Jarkko and Dave to comment on this, and include your "Tested-by".
> 
> I will also add cond_resched() on the final submission.

Why these would be conflicting tasks? I.e. why could not QEMU use
what is available now and move forward using better mechanism, when
they are available?

BTW, I do all my SGX testing ATM in QEMU (for some weeks). IMHO, it's
already "good enough" for many tasks, even if this fallback case is
not perfectly sorted out.

/Jarkko

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

[Paolo Bonzini]

On 14/09/21 18:42, Jarkko Sakkinen wrote:
>> Let's wait for this patch to be accepted first.  I'll wait a little more
>> for Jarkko and Dave to comment on this, and include your "Tested-by".
>>
>> I will also add cond_resched() on the final submission.
> Why these would be conflicting tasks? I.e. why could not QEMU use
> what is available now and move forward using better mechanism, when
> they are available?

The implementation using close/open is quite ugly (destroying and 
recreating the memory block breaks a few levels of abstractions), so 
it's not really something I'd like to commit.

Paolo

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

[Jarkko Sakkinen]

On Tue, 2021-09-14 at 19:07 +0200, Paolo Bonzini wrote:
> On 14/09/21 18:42, Jarkko Sakkinen wrote:
> > > Let's wait for this patch to be accepted first.  I'll wait a little more
> > > for Jarkko and Dave to comment on this, and include your "Tested-by".
> > > 
> > > I will also add cond_resched() on the final submission.
> > Why these would be conflicting tasks? I.e. why could not QEMU use
> > what is available now and move forward using better mechanism, when
> > they are available?
> 
> The implementation using close/open is quite ugly (destroying and 
> recreating the memory block breaks a few levels of abstractions), so 
> it's not really something I'd like to commit.

OK, so the driving reason for SGX_IOC_VEPC_RESET is the complex dance
with opening, closing and mmapping() stuff, especially when dealing
with multiple sections for one VM? OK, I think I can understand this,
given how notorious it might be to get stable in the user space.

Please just document this use case some way (if I got it right) to
the commit message of the next version, and I think this starts to
make much more sense.

/Jarkko

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

[Jarkko Sakkinen]

On Tue, 2021-09-14 at 20:40 +0300, Jarkko Sakkinen wrote:
> On Tue, 2021-09-14 at 19:07 +0200, Paolo Bonzini wrote:
> > On 14/09/21 18:42, Jarkko Sakkinen wrote:
> > > > Let's wait for this patch to be accepted first.  I'll wait a little more
> > > > for Jarkko and Dave to comment on this, and include your "Tested-by".
> > > > 
> > > > I will also add cond_resched() on the final submission.
> > > Why these would be conflicting tasks? I.e. why could not QEMU use
> > > what is available now and move forward using better mechanism, when
> > > they are available?
> > 
> > The implementation using close/open is quite ugly (destroying and 
> > recreating the memory block breaks a few levels of abstractions), so 
> > it's not really something I'd like to commit.
> 
> OK, so the driving reason for SGX_IOC_VEPC_RESET is the complex dance
> with opening, closing and mmapping() stuff, especially when dealing
> with multiple sections for one VM? OK, I think I can understand this,
> given how notorious it might be to get stable in the user space.
> 
> Please just document this use case some way (if I got it right) to
> the commit message of the next version, and I think this starts to
> make much more sense.

I would call it bottleneck rather than race though. I would keep
race for the things that can cause real race condition inside the
kernel corrupting data structures or whatever.

But for sure it is bottleneck that can easily cause user space to
be racy without extra-ordinary carefulness.

/Jarkko

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

[Yang Zhong]

On Tue, Sep 14, 2021 at 12:19:31PM +0200, Paolo Bonzini wrote:
> On 14/09/21 09:10, Yang Zhong wrote:
> >On Mon, Sep 13, 2021 at 09:11:51AM -0400, Paolo Bonzini wrote:
> >>Based on discussions from the previous week(end), this series implements
> >>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.
> >>
> >>The patches are untested, but I am posting them because they are simple
> >>and so that Yang Zhong can try using them in QEMU.
> >>
> >
> >   Paolo, i re-implemented one reset patch in the Qemu side to call this ioctl(),
> >   and did some tests on Windows and Linux guest, the Windows/Linux guest reboot
> >   work well.
> >
> >   So, it is time for me to send this reset patch to Qemu community? or wait for
> >   this kernel patchset merged? thanks!
> 
> Let's wait for this patch to be accepted first.  I'll wait a little
> more for Jarkko and Dave to comment on this, and include your
> "Tested-by".
> 
> I will also add cond_resched() on the final submission.
> 

  Thanks Paolo, i will send Qemu patch once this patchset is accepted.

  This day, i also did corner cases test and updated related Qemu reset patch.
   
   do {
       ret = ioctl(fd, SGX_IOC_VEPC_REMOVE);
       /* this printf is only for debug*/
       printf("-------sgx ret=%d and n=%d---\n", ret, n++);
       if(ret)
           sleep(1);
   } while (ret);  

  (1). The VEPC size=10M, start 4 enclaves(each ~2G size) in the VM side.
       then do the 'system_reset' in the Qemu monitor tool.
       
  (2). The VEPC size=10G, start 500 enclaves(each ~20M size) in the VM side.
       then do the 'system_reset' in the Qemu monitor tool.

  The ret will show the failures number(SECS pages number, 4 and 500) got from kernel side,
  after sleep 1s, the ioctl will return 0 failures.

  If this reset is triggered by guest bios, there is 0 SECS page got from kernel, which will
  not block VM booting.

  So, until now, the kernel patches work well. If any new issue, i will update it to all. thanks!      

  Yang

> Paolo