[PATCH] x86/sgx: Add SGX_PAGE_REPEAT flag for SGX_IOC_ENCLAVE_ADD_PAGES

[PATCH] x86/sgx: Add SGX_PAGE_REPEAT flag for SGX_IOC_ENCLAVE_ADD_PAGES

[Jarkko Sakkinen]

For uninitialized data, there's a need to add the same page multiple times,
e.g. a zero page, instead of traversing the source memory forward. With the
current API, this requires to call SGX_IOC_ENCLAVE_ADD_PAGES multiple
times, once per page, which is not very efficient.

Add a new SGX_PAGE_REPEAT flag to resolve the issue. When this flag is set
to the 'flags' field of struct sgx_enclave_pages, the ioctl will apply the
page at 'src' multiple times, instead of moving forward in the address
space.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 arch/x86/include/uapi/asm/sgx.h | 3 +++
 arch/x86/kernel/cpu/sgx/ioctl.c | 9 +++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 9690d6899ad9..d20fd54fa250 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -12,9 +12,12 @@
  * enum sgx_page_flags - page control flags
  * %SGX_PAGE_MEASURE:	Measure the page contents with a sequence of
  *			ENCLS[EEXTEND] operations.
+ * %SGX_PAGE_REPEAT:	Read the same page at 'src' multiple times,
+ *			instead of traversing the memory forward.
  */
 enum sgx_page_flags {
 	SGX_PAGE_MEASURE	= 0x01,
+	SGX_PAGE_REPEAT		= 0x02,
 };
 
 #define SGX_MAGIC 0xA4
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index 83df20e3e633..d81bb257bad0 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -415,6 +415,7 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
 {
 	struct sgx_enclave_add_pages add_arg;
 	struct sgx_secinfo secinfo;
+	unsigned long src;
 	unsigned long c;
 	int ret;
 
@@ -453,8 +454,12 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
 		if (need_resched())
 			cond_resched();
 
-		ret = sgx_encl_add_page(encl, add_arg.src + c, add_arg.offset + c,
-					&secinfo, add_arg.flags);
+		if (add_arg.flags & SGX_PAGE_REPEAT)
+			src = add_arg.src;
+		else
+			src = add_arg.src + c;
+
+		ret = sgx_encl_add_page(encl, src, add_arg.offset + c, &secinfo, add_arg.flags);
 		if (ret)
 			break;
 	}
-- 
2.31.1

Re: [PATCH] x86/sgx: Add SGX_PAGE_REPEAT flag for SGX_IOC_ENCLAVE_ADD_PAGES

[Jarkko Sakkinen]

On Thu, Jun 10, 2021 at 10:21:17AM +0300, Jarkko Sakkinen wrote:
> For uninitialized data, there's a need to add the same page multiple times,
> e.g. a zero page, instead of traversing the source memory forward. With the
> current API, this requires to call SGX_IOC_ENCLAVE_ADD_PAGES multiple
> times, once per page, which is not very efficient.
> 
> Add a new SGX_PAGE_REPEAT flag to resolve the issue. When this flag is set
> to the 'flags' field of struct sgx_enclave_pages, the ioctl will apply the
> page at 'src' multiple times, instead of moving forward in the address
> space.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

After sending this, I started to think that maybe it would actually better
to just add SGX_PAGE_ZERO flag, i.e. add zero pages and ignore src. That's
the main use case right now, and saves the user space from extra trouble of
having to do such page by hand.

That neither does prevent adding SGX_PAGE_REPEAT later on. I just see no
point of that generic functionality right now. It only makes simple use
case more complex.

/Jarkko

Re: [PATCH] x86/sgx: Add SGX_PAGE_REPEAT flag for SGX_IOC_ENCLAVE_ADD_PAGES

[Dave Hansen]

On 6/10/21 2:01 AM, Jarkko Sakkinen wrote:
> On Thu, Jun 10, 2021 at 10:21:17AM +0300, Jarkko Sakkinen wrote:
>> For uninitialized data, there's a need to add the same page multiple times,
>> e.g. a zero page, instead of traversing the source memory forward. With the
>> current API, this requires to call SGX_IOC_ENCLAVE_ADD_PAGES multiple
>> times, once per page, which is not very efficient.
>>
>> Add a new SGX_PAGE_REPEAT flag to resolve the issue. When this flag is set
>> to the 'flags' field of struct sgx_enclave_pages, the ioctl will apply the
>> page at 'src' multiple times, instead of moving forward in the address
>> space.
>>
>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> After sending this, I started to think that maybe it would actually better
> to just add SGX_PAGE_ZERO flag, i.e. add zero pages and ignore src. That's
> the main use case right now, and saves the user space from extra trouble of
> having to do such page by hand.
> 
> That neither does prevent adding SGX_PAGE_REPEAT later on. I just see no
> point of that generic functionality right now. It only makes simple use
> case more complex.

Is the main argument behind this new ABI that it increases efficiency?

Let's say I want to add 1MB of 0'd pages to an enclave.  Won't this do it?

	zeros = mmap(NULL, size, PROT_READ, MAP_ANONYMOUS|MAP_PRIVATE,
		     -1, 0);
	ioctl(SGX_IOC_ENCLAVE_ADD_PAGES, zeros, size);

Sure, you'll pay the cost of faulting in the zero page size/PAGE_SIZE
times.  But, that's pretty minuscule.  This zeros buffer can also be
reused without faulting again.  It can be as big or small as you want.
Heck, it could even be 2MB in size and use the transparent huge page.

I agree that there's definitely some optimization work to do.  But, I'm
a bit hesitant to turn to new ABI to do it.

Re: [PATCH] x86/sgx: Add SGX_PAGE_REPEAT flag for SGX_IOC_ENCLAVE_ADD_PAGES

[Jarkko Sakkinen]

On Thu, Jun 10, 2021 at 07:11:53AM -0700, Dave Hansen wrote:
> On 6/10/21 2:01 AM, Jarkko Sakkinen wrote:
> > On Thu, Jun 10, 2021 at 10:21:17AM +0300, Jarkko Sakkinen wrote:
> >> For uninitialized data, there's a need to add the same page multiple times,
> >> e.g. a zero page, instead of traversing the source memory forward. With the
> >> current API, this requires to call SGX_IOC_ENCLAVE_ADD_PAGES multiple
> >> times, once per page, which is not very efficient.
> >>
> >> Add a new SGX_PAGE_REPEAT flag to resolve the issue. When this flag is set
> >> to the 'flags' field of struct sgx_enclave_pages, the ioctl will apply the
> >> page at 'src' multiple times, instead of moving forward in the address
> >> space.
> >>
> >> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > After sending this, I started to think that maybe it would actually better
> > to just add SGX_PAGE_ZERO flag, i.e. add zero pages and ignore src. That's
> > the main use case right now, and saves the user space from extra trouble of
> > having to do such page by hand.
> > 
> > That neither does prevent adding SGX_PAGE_REPEAT later on. I just see no
> > point of that generic functionality right now. It only makes simple use
> > case more complex.
> 
> Is the main argument behind this new ABI that it increases efficiency?
> 
> Let's say I want to add 1MB of 0'd pages to an enclave.  Won't this do it?
> 
> 	zeros = mmap(NULL, size, PROT_READ, MAP_ANONYMOUS|MAP_PRIVATE,
> 		     -1, 0);
> 	ioctl(SGX_IOC_ENCLAVE_ADD_PAGES, zeros, size);

Later, with LSM's, it's better to mmap /dev/null.

> Sure, you'll pay the cost of faulting in the zero page size/PAGE_SIZE
> times.  But, that's pretty minuscule.  This zeros buffer can also be
> reused without faulting again.  It can be as big or small as you want.
> Heck, it could even be 2MB in size and use the transparent huge page.
> 
> I agree that there's definitely some optimization work to do.  But, I'm
> a bit hesitant to turn to new ABI to do it.

I realized that it's good to create heap memory like in your example
because then any possible SGX_IOC_ENCLAVE_ADD_PAGES invocation map to LSM
rules, i.e. every call has a well-defined source.

/Jarkko