linux-sgx.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH for_v23 0/7] x86/sgx: Improve add pages ioctl
@ 2019-10-09  4:42 Sean Christopherson
  2019-10-09  4:42 ` [PATCH for_v23 1/7] x86/sgx: Modify ADD_PAGE ioctl to take offset instead of full address Sean Christopherson
                   ` (7 more replies)
  0 siblings, 8 replies; 18+ messages in thread
From: Sean Christopherson @ 2019-10-09  4:42 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx

Enhance the SGX_IOC_ENCLAVE_ADD_PAGE{S} ioctl so that userspace can add
multiple pages to an enclave in a single syscall.  Also provide a flag
that allows replicating a single source page to multiple target pages so
that userspace doesn't need to allocate a giant chunk of memory when
initializing things like the enlave's .bss, heap, etc...

People that actually develop runtimes, please weigh in.  Jarkko also
suggested going with a fully flexible ioctl, e.g. essentially creating an
array of the existing struct so that mrmask and/or secinfo can be unique
per page.  AFAICT that's overkill and more cumbersome to use as it forces
userspace to allocate the full array.  My understanding is that the
majority of enclaves will have contiguous blocks of pages with identical
mrmask and secinfo, e.g. code segments, ro data, etc..., thus the less
flexible but easier-in-theory to use approach proposed here.

Sean Christopherson (7):
  x86/sgx: Modify ADD_PAGE ioctl to take offset instead of full address
  selftests/x86/sgx: Update test to account for ADD_PAGE change
  x86/sgx: Tweak ADD_PAGE ioctl to allow adding multiple pages
  selftests/x86/sgx: Update enclave build flow to do multi-page add
  x86/sgx: Add a flag to ADD_PAGES to allow replicating the source page
  selftests/x86/sgx: Update selftest to account for ADD_PAGES flag
  selftests/x86/sgx: Add test coverage for reclaim and replicate

 arch/x86/include/uapi/asm/sgx.h           | 25 +++++---
 arch/x86/kernel/cpu/sgx/ioctl.c           | 77 +++++++++++++++++------
 tools/testing/selftests/x86/sgx/defines.h | 28 +++++++++
 tools/testing/selftests/x86/sgx/main.c    | 40 ++++++------
 tools/testing/selftests/x86/sgx/sgxsign.c | 20 +++++-
 5 files changed, 140 insertions(+), 50 deletions(-)

-- 
2.22.0


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH for_v23 1/7] x86/sgx: Modify ADD_PAGE ioctl to take offset instead of full address
  2019-10-09  4:42 [PATCH for_v23 0/7] x86/sgx: Improve add pages ioctl Sean Christopherson
@ 2019-10-09  4:42 ` Sean Christopherson
  2019-10-09  4:42 ` [PATCH for_v23 2/7] selftests/x86/sgx: Update test to account for ADD_PAGE change Sean Christopherson
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 18+ messages in thread
From: Sean Christopherson @ 2019-10-09  4:42 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx

Change SGX_IOC_ENCLAVE_ADD_PAGE to take the target page as an offset
instead of a full address now that the API no longer uses the address to
find the target enclave.

Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/uapi/asm/sgx.h |  4 ++--
 arch/x86/kernel/cpu/sgx/ioctl.c | 10 +++++-----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 8f4660e07f6b..67583b046af1 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -31,14 +31,14 @@ struct sgx_enclave_create  {
 /**
  * struct sgx_enclave_add_page - parameter structure for the
  *                               %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
- * @addr:	address within the ELRANGE
+ * @offset:	page offset within the enclave
  * @src:	address for the page data
  * @secinfo:	address for the SECINFO data
  * @mrmask:	bitmask for the measured 256 byte chunks
  * @reserved:	reserved for future use
  */
 struct sgx_enclave_add_page {
-	__u64	addr;
+	__u64	offset;
 	__u64	src;
 	__u64	secinfo;
 	__u16	mrmask;
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index 75f868bad3ea..f407dd35f9e3 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -113,7 +113,7 @@ static int sgx_validate_secs(const struct sgx_secs *secs,
 }
 
 static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
-						 unsigned long addr,
+						 unsigned long offset,
 						 u64 secinfo_flags)
 {
 	struct sgx_encl_page *encl_page;
@@ -123,7 +123,7 @@ static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
 	if (!encl_page)
 		return ERR_PTR(-ENOMEM);
 
-	encl_page->desc = addr;
+	encl_page->desc = encl->base + offset;
 	encl_page->encl = encl;
 
 	prot = _calc_vm_trans(secinfo_flags, SGX_SECINFO_R, PROT_READ)  |
@@ -368,7 +368,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl,
 	struct sgx_va_page *va_page;
 	int ret;
 
-	encl_page = sgx_encl_page_alloc(encl, addp->addr, secinfo->flags);
+	encl_page = sgx_encl_page_alloc(encl, addp->offset, secinfo->flags);
 	if (IS_ERR(encl_page))
 		return PTR_ERR(encl_page);
 
@@ -485,11 +485,11 @@ static long sgx_ioc_enclave_add_page(struct sgx_encl *encl, void __user *arg)
 	if (copy_from_user(&addp, arg, sizeof(addp)))
 		return -EFAULT;
 
-	if (!IS_ALIGNED(addp.addr, PAGE_SIZE) ||
+	if (!IS_ALIGNED(addp.offset, PAGE_SIZE) ||
 	    !IS_ALIGNED(addp.src, PAGE_SIZE))
 		return -EINVAL;
 
-	if (addp.addr < encl->base || addp.addr - encl->base >= encl->size)
+	if (addp.offset >= encl->size)
 		return -EINVAL;
 
 	if (copy_from_user(&secinfo, (void __user *)addp.secinfo,
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH for_v23 2/7] selftests/x86/sgx: Update test to account for ADD_PAGE change
  2019-10-09  4:42 [PATCH for_v23 0/7] x86/sgx: Improve add pages ioctl Sean Christopherson
  2019-10-09  4:42 ` [PATCH for_v23 1/7] x86/sgx: Modify ADD_PAGE ioctl to take offset instead of full address Sean Christopherson
@ 2019-10-09  4:42 ` Sean Christopherson
  2019-10-09  4:42 ` [PATCH for_v23 3/7] x86/sgx: Tweak ADD_PAGE ioctl to allow adding multiple pages Sean Christopherson
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 18+ messages in thread
From: Sean Christopherson @ 2019-10-09  4:42 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx

Update the SGX selftest to pass the target offset instead of the full
address when adding pages to an enclave.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/x86/sgx/main.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index f78ff458b0dd..7cffc16c02e0 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -164,7 +164,7 @@ static bool encl_create(int dev_fd, unsigned long bin_size,
 	return true;
 }
 
-static bool encl_add_page(int dev_fd, unsigned long addr, void *data,
+static bool encl_add_page(int dev_fd, unsigned long offset, void *data,
 			  uint64_t flags)
 {
 	struct sgx_enclave_add_page ioc;
@@ -176,7 +176,7 @@ static bool encl_add_page(int dev_fd, unsigned long addr, void *data,
 
 	ioc.secinfo = (unsigned long)&secinfo;
 	ioc.mrmask = 0xFFFF;
-	ioc.addr = addr;
+	ioc.offset = offset;
 	ioc.src = (uint64_t)data;
 	memset(ioc.reserved, 0, sizeof(ioc.reserved));
 
@@ -215,8 +215,7 @@ static bool encl_build(struct sgx_secs *secs, void *bin,
 			flags = SGX_SECINFO_REG | SGX_SECINFO_R |
 				SGX_SECINFO_W | SGX_SECINFO_X;
 
-		if (!encl_add_page(dev_fd, secs->base + offset,
-				   bin + offset, flags))
+		if (!encl_add_page(dev_fd, offset, bin + offset, flags))
 			goto out_map;
 	}
 
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH for_v23 3/7] x86/sgx: Tweak ADD_PAGE ioctl to allow adding multiple pages
  2019-10-09  4:42 [PATCH for_v23 0/7] x86/sgx: Improve add pages ioctl Sean Christopherson
  2019-10-09  4:42 ` [PATCH for_v23 1/7] x86/sgx: Modify ADD_PAGE ioctl to take offset instead of full address Sean Christopherson
  2019-10-09  4:42 ` [PATCH for_v23 2/7] selftests/x86/sgx: Update test to account for ADD_PAGE change Sean Christopherson
@ 2019-10-09  4:42 ` Sean Christopherson
  2019-10-14 21:32   ` Jarkko Sakkinen
  2019-10-09  4:42 ` [PATCH for_v23 4/7] selftests/x86/sgx: Update enclave build flow to do multi-page add Sean Christopherson
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 18+ messages in thread
From: Sean Christopherson @ 2019-10-09  4:42 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx

Add a nr_pages param to the ioctl for adding pages to the enclave so
that userspace can batch multiple pages in a single syscall.  Update the
offset, src and nr_pages params prior to returning to userspace so that
the caller has sufficient information to analyze failures and can easily
restart the ioctl when appropriate.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/uapi/asm/sgx.h | 16 ++++----
 arch/x86/kernel/cpu/sgx/ioctl.c | 68 +++++++++++++++++++++++++--------
 2 files changed, 61 insertions(+), 23 deletions(-)

diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 67583b046af1..84734229d8dd 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -12,8 +12,8 @@
 
 #define SGX_IOC_ENCLAVE_CREATE \
 	_IOW(SGX_MAGIC, 0x00, struct sgx_enclave_create)
-#define SGX_IOC_ENCLAVE_ADD_PAGE \
-	_IOW(SGX_MAGIC, 0x01, struct sgx_enclave_add_page)
+#define SGX_IOC_ENCLAVE_ADD_PAGES \
+	_IOWR(SGX_MAGIC, 0x01, struct sgx_enclave_add_pages)
 #define SGX_IOC_ENCLAVE_INIT \
 	_IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
 #define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \
@@ -29,17 +29,19 @@ struct sgx_enclave_create  {
 };
 
 /**
- * struct sgx_enclave_add_page - parameter structure for the
- *                               %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
- * @offset:	page offset within the enclave
- * @src:	address for the page data
+ * struct sgx_enclave_add_pages - parameter structure for the
+ *                                %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
+ * @offset:	starting page offset within the ELRANGE
+ * @src:	start address for the page data
+ * @nr_pages:	number of pages to add to enclave
  * @secinfo:	address for the SECINFO data
  * @mrmask:	bitmask for the measured 256 byte chunks
  * @reserved:	reserved for future use
  */
-struct sgx_enclave_add_page {
+struct sgx_enclave_add_pages {
 	__u64	offset;
 	__u64	src;
+	__u64	nr_pages;
 	__u64	secinfo;
 	__u16	mrmask;
 	__u8	reserved[6];
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index f407dd35f9e3..4597dd8f5c91 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -360,7 +360,7 @@ static int __sgx_encl_extend(struct sgx_encl *encl,
 }
 
 static int sgx_encl_add_page(struct sgx_encl *encl,
-			     struct sgx_enclave_add_page *addp,
+			     struct sgx_enclave_add_pages *addp,
 			     struct sgx_secinfo *secinfo)
 {
 	struct sgx_encl_page *encl_page;
@@ -443,14 +443,19 @@ static int sgx_encl_add_page(struct sgx_encl *encl,
 }
 
 /**
- * sgx_ioc_enclave_add_page() - The handler for %SGX_IOC_ENCLAVE_ADD_PAGE
- * @filep:	open file to /dev/sgx
- * @arg:	a user pointer to a struct sgx_enclave_add_page instance
+ * sgx_ioc_enclave_add_pages() - The handler for %SGX_IOC_ENCLAVE_ADD_PAGES
+ * @encl:       pointer to an enclave instance (via ioctl() file pointer)
+ * @arg:	a user pointer to a struct sgx_enclave_add_pages instance
  *
- * Add (EADD) a page to an uninitialized enclave, and optionally extend
- * (EEXTEND) the measurement with the contents of the page. A SECINFO for a TCS
- * is required to always contain zero permissions because CPU silently zeros
- * them. Allowing anything else would cause a mismatch in the measurement.
+ * Add (EADD) one or more pages to an uninitialized enclave, and optionally
+ * extend (EEXTEND) the measurement with the contents of the page. The range of
+ * pages must be virtually contiguous. The SECINFO and measurement mask are
+ * applied to all pages, i.e. pages with different properties must be added in
+ * separate calls.
+ *
+ * A SECINFO for a TCS is required to always contain zero permissions because
+ * CPU silently zeros them. Allowing anything else would cause a mismatch in
+ * the measurement.
  *
  * mmap()'s protection bits are capped by the page permissions. For each page
  * address, the maximum protection bits are computed with the following
@@ -467,17 +472,25 @@ static int sgx_encl_add_page(struct sgx_encl *encl,
  * permissions. In effect, this allows mmap() with PROT_NONE to be used to seek
  * an address range for the enclave that can be then populated into SECS.
  *
+ * @arg->addr, @arg->src and @arg->nr_pages are adjusted to reflect the
+ * remaining pages that need to be added to the enclave, e.g. userspace can
+ * re-invoke SGX_IOC_ENCLAVE_ADD_PAGES using the same struct in response to an
+ * ERESTARTSYS error.
+ *
  * Return:
  *   0 on success,
- *   -EINVAL if the SECINFO contains invalid data,
- *   -EACCES if the source page is located in a noexec partition,
+ *   -EINVAL if any input param or the SECINFO contains invalid data,
+ *   -EACCES if an executable source page is located in a noexec partition,
  *   -ENOMEM if any memory allocation, including EPC, fails,
+ *   -ERESTARTSYS if a pending signal is recognized,
  *   -errno otherwise
+
  */
-static long sgx_ioc_enclave_add_page(struct sgx_encl *encl, void __user *arg)
+static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
 {
-	struct sgx_enclave_add_page addp;
+	struct sgx_enclave_add_pages addp;
 	struct sgx_secinfo secinfo;
+	int ret;
 
 	if (!(atomic_read(&encl->flags) & SGX_ENCL_CREATED))
 		return -EINVAL;
@@ -489,7 +502,10 @@ static long sgx_ioc_enclave_add_page(struct sgx_encl *encl, void __user *arg)
 	    !IS_ALIGNED(addp.src, PAGE_SIZE))
 		return -EINVAL;
 
-	if (addp.offset >= encl->size)
+	if (!addp.nr_pages)
+		return -EINVAL;
+
+	if (addp.offset + ((addp.nr_pages - 1) * PAGE_SIZE) >= encl->size)
 		return -EINVAL;
 
 	if (copy_from_user(&secinfo, (void __user *)addp.secinfo,
@@ -499,7 +515,27 @@ static long sgx_ioc_enclave_add_page(struct sgx_encl *encl, void __user *arg)
 	if (sgx_validate_secinfo(&secinfo))
 		return -EINVAL;
 
-	return sgx_encl_add_page(encl, &addp, &secinfo);
+	for ( ; addp.nr_pages > 0; addp.nr_pages--) {
+		if (signal_pending(current)) {
+			ret = -ERESTARTSYS;
+			break;
+		}
+
+		if (need_resched())
+			cond_resched();
+
+		ret = sgx_encl_add_page(encl, &addp, &secinfo);
+		if (ret)
+			break;
+
+		addp.offset += PAGE_SIZE;
+		addp.src += PAGE_SIZE;
+	}
+
+	if (copy_to_user(arg, &addp, sizeof(addp)))
+		return -EFAULT;
+
+	return ret;
 }
 
 static int __sgx_get_key_hash(struct crypto_shash *tfm, const void *modulus,
@@ -702,8 +738,8 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 	case SGX_IOC_ENCLAVE_CREATE:
 		ret = sgx_ioc_enclave_create(encl, (void __user *)arg);
 		break;
-	case SGX_IOC_ENCLAVE_ADD_PAGE:
-		ret = sgx_ioc_enclave_add_page(encl, (void __user *)arg);
+	case SGX_IOC_ENCLAVE_ADD_PAGES:
+		ret = sgx_ioc_enclave_add_pages(encl, (void __user *)arg);
 		break;
 	case SGX_IOC_ENCLAVE_INIT:
 		ret = sgx_ioc_enclave_init(encl, (void __user *)arg);
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH for_v23 4/7] selftests/x86/sgx: Update enclave build flow to do multi-page add
  2019-10-09  4:42 [PATCH for_v23 0/7] x86/sgx: Improve add pages ioctl Sean Christopherson
                   ` (2 preceding siblings ...)
  2019-10-09  4:42 ` [PATCH for_v23 3/7] x86/sgx: Tweak ADD_PAGE ioctl to allow adding multiple pages Sean Christopherson
@ 2019-10-09  4:42 ` Sean Christopherson
  2019-10-09  4:42 ` [PATCH for_v23 5/7] x86/sgx: Add a flag to ADD_PAGES to allow replicating the source page Sean Christopherson
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 18+ messages in thread
From: Sean Christopherson @ 2019-10-09  4:42 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx

Add the enclaves regular pages in a single ioctl to test the multi-page
capabilities of the ioctl.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/x86/sgx/main.c | 27 +++++++++++---------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index 7cffc16c02e0..4b652fd800f5 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -164,10 +164,10 @@ static bool encl_create(int dev_fd, unsigned long bin_size,
 	return true;
 }
 
-static bool encl_add_page(int dev_fd, unsigned long offset, void *data,
-			  uint64_t flags)
+static bool encl_add_pages(int dev_fd, unsigned long offset, void *data,
+			   unsigned long nr_pages, uint64_t flags)
 {
-	struct sgx_enclave_add_page ioc;
+	struct sgx_enclave_add_pages ioc;
 	struct sgx_secinfo secinfo;
 	int rc;
 
@@ -178,9 +178,10 @@ static bool encl_add_page(int dev_fd, unsigned long offset, void *data,
 	ioc.mrmask = 0xFFFF;
 	ioc.offset = offset;
 	ioc.src = (uint64_t)data;
+	ioc.nr_pages = nr_pages;
 	memset(ioc.reserved, 0, sizeof(ioc.reserved));
 
-	rc = ioctl(dev_fd, SGX_IOC_ENCLAVE_ADD_PAGE, &ioc);
+	rc = ioctl(dev_fd, SGX_IOC_ENCLAVE_ADD_PAGES, &ioc);
 	if (rc) {
 		fprintf(stderr, "EADD failed rc=%d.\n", rc);
 		return false;
@@ -189,12 +190,13 @@ static bool encl_add_page(int dev_fd, unsigned long offset, void *data,
 	return true;
 }
 
+#define SGX_REG_PAGE_FLAGS \
+	(SGX_SECINFO_REG | SGX_SECINFO_R | SGX_SECINFO_W | SGX_SECINFO_X)
+
 static bool encl_build(struct sgx_secs *secs, void *bin,
 		       unsigned long bin_size, struct sgx_sigstruct *sigstruct)
 {
 	struct sgx_enclave_init ioc;
-	uint64_t offset;
-	uint64_t flags;
 	void *addr;
 	int dev_fd;
 	int rc;
@@ -208,16 +210,9 @@ static bool encl_build(struct sgx_secs *secs, void *bin,
 	if (!encl_create(dev_fd, bin_size, secs))
 		goto out_dev_fd;
 
-	for (offset = 0; offset < bin_size; offset += 0x1000) {
-		if (!offset)
-			flags = SGX_SECINFO_TCS;
-		else
-			flags = SGX_SECINFO_REG | SGX_SECINFO_R |
-				SGX_SECINFO_W | SGX_SECINFO_X;
-
-		if (!encl_add_page(dev_fd, offset, bin + offset, flags))
-			goto out_map;
-	}
+	encl_add_pages(dev_fd, 0, bin, 1, SGX_SECINFO_TCS);
+	encl_add_pages(dev_fd, PAGE_SIZE, bin + PAGE_SIZE,
+		       (bin_size / PAGE_SIZE) - 1, SGX_REG_PAGE_FLAGS);
 
 	ioc.sigstruct = (uint64_t)sigstruct;
 	rc = ioctl(dev_fd, SGX_IOC_ENCLAVE_INIT, &ioc);
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH for_v23 5/7] x86/sgx: Add a flag to ADD_PAGES to allow replicating the source page
  2019-10-09  4:42 [PATCH for_v23 0/7] x86/sgx: Improve add pages ioctl Sean Christopherson
                   ` (3 preceding siblings ...)
  2019-10-09  4:42 ` [PATCH for_v23 4/7] selftests/x86/sgx: Update enclave build flow to do multi-page add Sean Christopherson
@ 2019-10-09  4:42 ` Sean Christopherson
  2019-10-09  4:42 ` [PATCH for_v23 6/7] selftests/x86/sgx: Update selftest to account for ADD_PAGES flag Sean Christopherson
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 18+ messages in thread
From: Sean Christopherson @ 2019-10-09  4:42 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx

Add a flag to allow userspace to replicate a single source page to
multiple target pages in the enclave, e.g. to zero the .bss, initialize
the heap, etc...

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/uapi/asm/sgx.h | 7 ++++++-
 arch/x86/kernel/cpu/sgx/ioctl.c | 3 ++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 84734229d8dd..42634e99945e 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -28,6 +28,9 @@ struct sgx_enclave_create  {
 	__u64	src;
 };
 
+/* Replicate a single source data page to all target pages. */
+#define SGX_ADD_PAGES_REPLICATE_SRC	BIT(0)
+
 /**
  * struct sgx_enclave_add_pages - parameter structure for the
  *                                %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
@@ -35,6 +38,7 @@ struct sgx_enclave_create  {
  * @src:	start address for the page data
  * @nr_pages:	number of pages to add to enclave
  * @secinfo:	address for the SECINFO data
+ * @flags:	misc control flags
  * @mrmask:	bitmask for the measured 256 byte chunks
  * @reserved:	reserved for future use
  */
@@ -43,8 +47,9 @@ struct sgx_enclave_add_pages {
 	__u64	src;
 	__u64	nr_pages;
 	__u64	secinfo;
+	__u32	flags;
 	__u16	mrmask;
-	__u8	reserved[6];
+	__u8	reserved[2];
 };
 
 /**
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index 4597dd8f5c91..9c6d582612cb 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -529,7 +529,8 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
 			break;
 
 		addp.offset += PAGE_SIZE;
-		addp.src += PAGE_SIZE;
+		if (!(addp.flags & SGX_ADD_PAGES_REPLICATE_SRC))
+			addp.src += PAGE_SIZE;
 	}
 
 	if (copy_to_user(arg, &addp, sizeof(addp)))
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH for_v23 6/7] selftests/x86/sgx: Update selftest to account for ADD_PAGES flag
  2019-10-09  4:42 [PATCH for_v23 0/7] x86/sgx: Improve add pages ioctl Sean Christopherson
                   ` (4 preceding siblings ...)
  2019-10-09  4:42 ` [PATCH for_v23 5/7] x86/sgx: Add a flag to ADD_PAGES to allow replicating the source page Sean Christopherson
@ 2019-10-09  4:42 ` Sean Christopherson
  2019-10-09  4:42 ` [PATCH for_v23 7/7] selftests/x86/sgx: Add test coverage for reclaim and replicate Sean Christopherson
  2019-10-10  3:28 ` [PATCH for_v23 0/7] x86/sgx: Improve add pages ioctl Haitao Huang
  7 siblings, 0 replies; 18+ messages in thread
From: Sean Christopherson @ 2019-10-09  4:42 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx

Update the call to SGX_IOC_ENCLAVE_ADD_PAGES to pass '0' for the flags,
i.e. retain the existing behavior.

No functional change intended.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/x86/sgx/main.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index 4b652fd800f5..0921aeda9942 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -165,20 +165,22 @@ static bool encl_create(int dev_fd, unsigned long bin_size,
 }
 
 static bool encl_add_pages(int dev_fd, unsigned long offset, void *data,
-			   unsigned long nr_pages, uint64_t flags)
+			   unsigned long nr_pages, uint64_t sec_flags,
+			   uint32_t misc_flags)
 {
 	struct sgx_enclave_add_pages ioc;
 	struct sgx_secinfo secinfo;
 	int rc;
 
 	memset(&secinfo, 0, sizeof(secinfo));
-	secinfo.flags = flags;
+	secinfo.flags = sec_flags;
 
 	ioc.secinfo = (unsigned long)&secinfo;
 	ioc.mrmask = 0xFFFF;
 	ioc.offset = offset;
 	ioc.src = (uint64_t)data;
 	ioc.nr_pages = nr_pages;
+	ioc.flags = misc_flags;
 	memset(ioc.reserved, 0, sizeof(ioc.reserved));
 
 	rc = ioctl(dev_fd, SGX_IOC_ENCLAVE_ADD_PAGES, &ioc);
@@ -210,9 +212,9 @@ static bool encl_build(struct sgx_secs *secs, void *bin,
 	if (!encl_create(dev_fd, bin_size, secs))
 		goto out_dev_fd;
 
-	encl_add_pages(dev_fd, 0, bin, 1, SGX_SECINFO_TCS);
+	encl_add_pages(dev_fd, 0, bin, 1, SGX_SECINFO_TCS, 0);
 	encl_add_pages(dev_fd, PAGE_SIZE, bin + PAGE_SIZE,
-		       (bin_size / PAGE_SIZE) - 1, SGX_REG_PAGE_FLAGS);
+		       (bin_size / PAGE_SIZE) - 1, SGX_REG_PAGE_FLAGS, 0);
 
 	ioc.sigstruct = (uint64_t)sigstruct;
 	rc = ioctl(dev_fd, SGX_IOC_ENCLAVE_INIT, &ioc);
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH for_v23 7/7] selftests/x86/sgx: Add test coverage for reclaim and replicate
  2019-10-09  4:42 [PATCH for_v23 0/7] x86/sgx: Improve add pages ioctl Sean Christopherson
                   ` (5 preceding siblings ...)
  2019-10-09  4:42 ` [PATCH for_v23 6/7] selftests/x86/sgx: Update selftest to account for ADD_PAGES flag Sean Christopherson
@ 2019-10-09  4:42 ` Sean Christopherson
  2019-10-10  3:28 ` [PATCH for_v23 0/7] x86/sgx: Improve add pages ioctl Haitao Huang
  7 siblings, 0 replies; 18+ messages in thread
From: Sean Christopherson @ 2019-10-09  4:42 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx

Pad 2*epc_size bytes to the end of the selftest enclave to test basic
reclaim functionality, and use the new replicate flag when adding the
pages.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/x86/sgx/defines.h | 28 +++++++++++++++++++++++
 tools/testing/selftests/x86/sgx/main.c    |  8 ++++++-
 tools/testing/selftests/x86/sgx/sgxsign.c | 20 ++++++++++++++--
 3 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/x86/sgx/defines.h b/tools/testing/selftests/x86/sgx/defines.h
index 3ff73a9d9b93..8d7b19b7e658 100644
--- a/tools/testing/selftests/x86/sgx/defines.h
+++ b/tools/testing/selftests/x86/sgx/defines.h
@@ -36,4 +36,32 @@ typedef uint64_t u64;
 #include "../../../../../arch/x86/kernel/cpu/sgx/arch.h"
 #include "../../../../../arch/x86/include/uapi/asm/sgx.h"
 
+/* Used to tack on unused data to the enclave to test reclaim and replicate. */
+#define SGX_SELFTEST_FILL_VALUE 0xcc
+
+static inline uint64_t get_epc_size(void)
+{
+	uint32_t eax, ebx, ecx, edx;
+	uint64_t size = 0;
+	int i;
+
+	for (i = 2; ; i++) {
+		asm volatile("cpuid"
+			     : "=a"(eax), "=b"(ebx), "=c"(ecx), "=d"(edx)
+			     : "a"(0x12), "c"(i));
+
+		if ((eax & SGX_CPUID_SUB_LEAF_TYPE_MASK) !=
+			SGX_CPUID_SUB_LEAF_EPC_SECTION)
+			break;
+
+		size += ((ecx & 0xfffff000UL) | ((uint64_t)edx << 32));
+	}
+	return size;
+}
+
+static inline uint64_t get_fill_size(void)
+{
+	return get_epc_size() * 2;
+}
+
 #endif /* TYPES_H */
diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index 0921aeda9942..d179b536d007 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -198,6 +198,8 @@ static bool encl_add_pages(int dev_fd, unsigned long offset, void *data,
 static bool encl_build(struct sgx_secs *secs, void *bin,
 		       unsigned long bin_size, struct sgx_sigstruct *sigstruct)
 {
+	uint8_t fill_page[PAGE_SIZE] __aligned(4096);
+	uint64_t fill_size = get_fill_size();
 	struct sgx_enclave_init ioc;
 	void *addr;
 	int dev_fd;
@@ -209,12 +211,16 @@ static bool encl_build(struct sgx_secs *secs, void *bin,
 		return false;
 	}
 
-	if (!encl_create(dev_fd, bin_size, secs))
+	if (!encl_create(dev_fd, bin_size + fill_size, secs))
 		goto out_dev_fd;
 
+	memset(fill_page, SGX_SELFTEST_FILL_VALUE, PAGE_SIZE);
+
 	encl_add_pages(dev_fd, 0, bin, 1, SGX_SECINFO_TCS, 0);
 	encl_add_pages(dev_fd, PAGE_SIZE, bin + PAGE_SIZE,
 		       (bin_size / PAGE_SIZE) - 1, SGX_REG_PAGE_FLAGS, 0);
+	encl_add_pages(dev_fd, bin_size, fill_page, fill_size / PAGE_SIZE,
+		       SGX_REG_PAGE_FLAGS, SGX_ADD_PAGES_REPLICATE_SRC);
 
 	ioc.sigstruct = (uint64_t)sigstruct;
 	rc = ioctl(dev_fd, SGX_IOC_ENCLAVE_INIT, &ioc);
diff --git a/tools/testing/selftests/x86/sgx/sgxsign.c b/tools/testing/selftests/x86/sgx/sgxsign.c
index 3d9007af40c9..98dee0d4b376 100644
--- a/tools/testing/selftests/x86/sgx/sgxsign.c
+++ b/tools/testing/selftests/x86/sgx/sgxsign.c
@@ -231,8 +231,9 @@ static bool measure_encl(const char *path, uint8_t *mrenclave)
 	struct stat sb;
 	EVP_MD_CTX *ctx;
 	uint64_t flags;
-	uint64_t offset;
+	uint64_t offset, i;
 	uint8_t data[0x1000];
+	uint64_t fill_size;
 	int rc;
 
 	ctx = EVP_MD_CTX_create();
@@ -257,7 +258,9 @@ static bool measure_encl(const char *path, uint8_t *mrenclave)
 		goto out;
 	}
 
-	if (!mrenclave_ecreate(ctx, sb.st_size))
+	fill_size = get_fill_size();
+
+	if (!mrenclave_ecreate(ctx, sb.st_size + fill_size))
 		goto out;
 
 	for (offset = 0; offset < sb.st_size; offset += 0x1000) {
@@ -280,6 +283,19 @@ static bool measure_encl(const char *path, uint8_t *mrenclave)
 			goto out;
 	}
 
+	memset(data, SGX_SELFTEST_FILL_VALUE, 0x1000);
+
+	for (i = 0; i < fill_size; i += 0x1000) {
+		flags = SGX_SECINFO_REG |
+			SGX_SECINFO_R | SGX_SECINFO_W | SGX_SECINFO_X;
+
+		if (!mrenclave_eadd(ctx, offset + i, flags))
+			goto out;
+
+		if (!mrenclave_eextend(ctx, offset + i, data))
+			goto out;
+	}
+
 	if (!mrenclave_commit(ctx, mrenclave))
 		goto out;
 
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH for_v23 0/7] x86/sgx: Improve add pages ioctl
  2019-10-09  4:42 [PATCH for_v23 0/7] x86/sgx: Improve add pages ioctl Sean Christopherson
                   ` (6 preceding siblings ...)
  2019-10-09  4:42 ` [PATCH for_v23 7/7] selftests/x86/sgx: Add test coverage for reclaim and replicate Sean Christopherson
@ 2019-10-10  3:28 ` Haitao Huang
  2019-10-11 14:37   ` Sean Christopherson
  7 siblings, 1 reply; 18+ messages in thread
From: Haitao Huang @ 2019-10-10  3:28 UTC (permalink / raw)
  To: Jarkko Sakkinen, Sean Christopherson; +Cc: linux-sgx

On Tue, 08 Oct 2019 23:42:34 -0500, Sean Christopherson  
<sean.j.christopherson@intel.com> wrote:

> Enhance the SGX_IOC_ENCLAVE_ADD_PAGE{S} ioctl so that userspace can add
> multiple pages to an enclave in a single syscall.  Also provide a flag
> that allows replicating a single source page to multiple target pages so
> that userspace doesn't need to allocate a giant chunk of memory when
> initializing things like the enlave's .bss, heap, etc...
>
> People that actually develop runtimes, please weigh in.  Jarkko also
> suggested going with a fully flexible ioctl, e.g. essentially creating an
> array of the existing struct so that mrmask and/or secinfo can be unique
> per page.  AFAICT that's overkill and more cumbersome to use as it forces
> userspace to allocate the full array.  My understanding is that the
> majority of enclaves will have contiguous blocks of pages with identical
> mrmask and secinfo, e.g. code segments, ro data, etc..., thus the less
> flexible but easier-in-theory to use approach proposed here.
>
We think using the same mask for all pages (solution in this patch set) is  
reasonable. Although it seems odd that all pages would apply the same  
mask, this allows enough flexibility we can foresee.

Another option acceptable to us (Intel SGX runtime) is to change it to a  
flag and have bit zero define whether the whole page is measured via  
EEXTEND. This is simpler and allows other bits reserved for future usages.  
However, it would fail any SGX runtime that is measuring partial page for  
optimization purposes.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH for_v23 0/7] x86/sgx: Improve add pages ioctl
  2019-10-10  3:28 ` [PATCH for_v23 0/7] x86/sgx: Improve add pages ioctl Haitao Huang
@ 2019-10-11 14:37   ` Sean Christopherson
  2019-10-13 15:15     ` Dr. Greg
  0 siblings, 1 reply; 18+ messages in thread
From: Sean Christopherson @ 2019-10-11 14:37 UTC (permalink / raw)
  To: Haitao Huang
  Cc: Jarkko Sakkinen, linux-sgx, Jethro Beekman, Dr. G.W. Wettstein

+cc Jethro and Greg

On Wed, Oct 09, 2019 at 10:28:36PM -0500, Haitao Huang wrote:
> On Tue, 08 Oct 2019 23:42:34 -0500, Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> 
> >Enhance the SGX_IOC_ENCLAVE_ADD_PAGE{S} ioctl so that userspace can add
> >multiple pages to an enclave in a single syscall.  Also provide a flag
> >that allows replicating a single source page to multiple target pages so
> >that userspace doesn't need to allocate a giant chunk of memory when
> >initializing things like the enlave's .bss, heap, etc...
> >
> >People that actually develop runtimes, please weigh in.  Jarkko also
> >suggested going with a fully flexible ioctl, e.g. essentially creating an
> >array of the existing struct so that mrmask and/or secinfo can be unique
> >per page.  AFAICT that's overkill and more cumbersome to use as it forces
> >userspace to allocate the full array.  My understanding is that the
> >majority of enclaves will have contiguous blocks of pages with identical
> >mrmask and secinfo, e.g. code segments, ro data, etc..., thus the less
> >flexible but easier-in-theory to use approach proposed here.
> >
> We think using the same mask for all pages (solution in this patch set) is
> reasonable. Although it seems odd that all pages would apply the same mask,
> this allows enough flexibility we can foresee.

Jethro, last time I brought this up you mentioned that it'd be nice to
have an array of pages[*] instead of the repeat-for-each-page concept.
Is there a use case where taking an array would provide a substantial
benefit to userspace?  Taking an array has downsides, and I think would
actually be worse for the vast majority of use cases.

E.g. for a contiguous chunk with the repeater approach

	struct sgx_enclave_add_pages ioc;
	struct sgx_secinfo secinfo;

	memset(&secinfo, 0, sizeof(secinfo));
	secinfo.flags = sec_flags;

	ioc.secinfo = (unsigned long)&secinfo;
	ioc.mrmask = 0xFFFF;
	ioc.offset = offset;
	ioc.src = (uint64_t)data;
	ioc.nr_pages = nr_pages;
	ioc.flags = misc_flags;
	memset(ioc.reserved, 0, sizeof(ioc.reserved));

vs. something like this for the array approach

	struct sgx_enclave_add_pages ioc;
	unsigned long i;

	array = malloc(sizeof(struct sgx_enclave_add_page_entry) * nr_pages);
	for (i = 0; i < nr_pages; i++) {
		memset(entries[i].secinfo, 0, sizeof(secinfo));
		entries[i].secinfo.flags = sec_flags;

		entries[i].mrmask = 0xFFFF;
		entries[i].offset = offset;
		entries[i].src = (uint64_t)data;
		entries[i].flags = misc_flags;
		memset(entries[i].reserved, 0, sizeof(entries[i].reserved));
	}

	ioc.nr_pages = nr_pages;
	ioc.page_array = (uint64_t)array;
	memset(ioc.reserved, 0, sizeof(ioc.reserved));

The loop is mildly annoying, but the real killer is the array allocation.
SECINFO is 64 bytes, which means each entry is 88 bytes or more, e.g. around
180kb to add an 8mb chunk of .bss or heap.

My intention is/was for the multi-page add to be an opportunistic
optimization, not a way to add all enclave pages in a single ioctl.

[*] https://patchwork.kernel.org/patch/10977721/#22699225

> Another option acceptable to us (Intel SGX runtime) is to change it to a
> flag and have bit zero define whether the whole page is measured via
> EEXTEND. This is simpler and allows other bits reserved for future usages.
> However, it would fail any SGX runtime that is measuring partial page for
> optimization purposes.

This can be an orthogonal change.  I agree it makes sense to drop mrmask
and instead have a SGX_ADD_PAGES_MEASURED flag to cover the whole page.
Hiding the 256-byte granualarity from userspace is a good idea as it's not
intrinsically tied to the SGX architecture and exists only because of
latency requirements.  And most of the kernel interfaces work on 4k
granularity.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH for_v23 0/7] x86/sgx: Improve add pages ioctl
  2019-10-11 14:37   ` Sean Christopherson
@ 2019-10-13 15:15     ` Dr. Greg
  0 siblings, 0 replies; 18+ messages in thread
From: Dr. Greg @ 2019-10-13 15:15 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Haitao Huang, Jarkko Sakkinen, linux-sgx, Jethro Beekman

On Fri, Oct 11, 2019 at 07:37:25AM -0700, Sean Christopherson wrote:

> +cc Jethro and Greg

Good morning, I hope everyone is having or has had a pleasant weekend.

> On Wed, Oct 09, 2019 at 10:28:36PM -0500, Haitao Huang wrote:
> > On Tue, 08 Oct 2019 23:42:34 -0500, Sean Christopherson
> > <sean.j.christopherson@intel.com> wrote:
> > 
> > >Enhance the SGX_IOC_ENCLAVE_ADD_PAGE{S} ioctl so that userspace can add
> > >multiple pages to an enclave in a single syscall.  Also provide a flag
> > >that allows replicating a single source page to multiple target pages so
> > >that userspace doesn't need to allocate a giant chunk of memory when
> > >initializing things like the enlave's .bss, heap, etc...
> > >
> > >People that actually develop runtimes, please weigh in.  Jarkko also
> > >suggested going with a fully flexible ioctl, e.g. essentially creating an
> > >array of the existing struct so that mrmask and/or secinfo can be unique
> > >per page.  AFAICT that's overkill and more cumbersome to use as it forces
> > >userspace to allocate the full array.  My understanding is that the
> > >majority of enclaves will have contiguous blocks of pages with identical
> > >mrmask and secinfo, e.g. code segments, ro data, etc..., thus the less
> > >flexible but easier-in-theory to use approach proposed here.
> > >
> > We think using the same mask for all pages (solution in this patch set) is
> > reasonable. Although it seems odd that all pages would apply the same mask,
> > this allows enough flexibility we can foresee.

> Jethro, last time I brought this up you mentioned that it'd be nice to
> have an array of pages[*] instead of the repeat-for-each-page concept.
> Is there a use case where taking an array would provide a substantial
> benefit to userspace?  Taking an array has downsides, and I think would
> actually be worse for the vast majority of use cases.
>
> ... [ Example code removed ] ...
>
> The loop is mildly annoying, but the real killer is the array allocation.
> SECINFO is 64 bytes, which means each entry is 88 bytes or more, e.g. around
> 180kb to add an 8mb chunk of .bss or heap.
> 
> My intention is/was for the multi-page add to be an opportunistic
> optimization, not a way to add all enclave pages in a single ioctl.
> 
> [*] https://patchwork.kernel.org/patch/10977721/#22699225

The simpler the better from our perspective.

We would use the 'one-shot' method to initialize a block of pages with
a set of common characteristics.  Essentially constructing an image of
enclave page characteristics in userspace in order to load an enclave
image isn't something that we would envision doing.

> > Another option acceptable to us (Intel SGX runtime) is to change it to a
> > flag and have bit zero define whether the whole page is measured via
> > EEXTEND. This is simpler and allows other bits reserved for future usages.
> > However, it would fail any SGX runtime that is measuring partial page for
> > optimization purposes.

> This can be an orthogonal change.  I agree it makes sense to drop
> mrmask and instead have a SGX_ADD_PAGES_MEASURED flag to cover the
> whole page.  Hiding the 256-byte granualarity from userspace is a
> good idea as it's not intrinsically tied to the SGX architecture and
> exists only because of latency requirements.  And most of the kernel
> interfaces work on 4k granularity.

Specifying the ability to measure an entire page is also a straight
forward simplicity optimization that we would embrace.

Have a good week.

Dr. Greg

As always,
Dr. Greg Wettstein, Ph.D, Worker
IDfusion, LLC               SGX secured infrastructure and
4206 N. 19th Ave.           autonomously self-defensive platforms.
Fargo, ND  58102
PH: 701-281-1686            EMAIL: greg@idfusion.net
------------------------------------------------------------------------------
"Thank heaven for startups; without them we'd never have any
 advances."
                                -- Seymour Cray

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH for_v23 3/7] x86/sgx: Tweak ADD_PAGE ioctl to allow adding multiple pages
  2019-10-09  4:42 ` [PATCH for_v23 3/7] x86/sgx: Tweak ADD_PAGE ioctl to allow adding multiple pages Sean Christopherson
@ 2019-10-14 21:32   ` Jarkko Sakkinen
  2019-10-14 21:35     ` Jarkko Sakkinen
  0 siblings, 1 reply; 18+ messages in thread
From: Jarkko Sakkinen @ 2019-10-14 21:32 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx

On Tue, Oct 08, 2019 at 09:42:37PM -0700, Sean Christopherson wrote:
> Add a nr_pages param to the ioctl for adding pages to the enclave so
> that userspace can batch multiple pages in a single syscall.  Update the
> offset, src and nr_pages params prior to returning to userspace so that
> the caller has sufficient information to analyze failures and can easily
> restart the ioctl when appropriate.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Please provide a more robust API. Now you decrease the robustness.

E.g.

struct sgx_enclave_add_page_desc {
        __u64   offset;
        __u16   mrmask;
        __u8    reserved[6];
};

struct sgx_enclave_add_page {
        __u64   src;
        __u64   secinfo;
        __u64   nr_pages;
        __u64   pages;
};

/Jarkko

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH for_v23 3/7] x86/sgx: Tweak ADD_PAGE ioctl to allow adding multiple pages
  2019-10-14 21:32   ` Jarkko Sakkinen
@ 2019-10-14 21:35     ` Jarkko Sakkinen
  2019-10-14 23:31       ` Sean Christopherson
  0 siblings, 1 reply; 18+ messages in thread
From: Jarkko Sakkinen @ 2019-10-14 21:35 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx

On Tue, Oct 15, 2019 at 12:32:55AM +0300, Jarkko Sakkinen wrote:
> On Tue, Oct 08, 2019 at 09:42:37PM -0700, Sean Christopherson wrote:
> > Add a nr_pages param to the ioctl for adding pages to the enclave so
> > that userspace can batch multiple pages in a single syscall.  Update the
> > offset, src and nr_pages params prior to returning to userspace so that
> > the caller has sufficient information to analyze failures and can easily
> > restart the ioctl when appropriate.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> Please provide a more robust API. Now you decrease the robustness.
> 
> E.g.
> 
> struct sgx_enclave_add_page_desc {
>         __u64   offset;
>         __u16   mrmask;
>         __u8    reserved[6];
> };
> 
> struct sgx_enclave_add_page {
>         __u64   src;
>         __u64   secinfo;
>         __u64   nr_pages;
>         __u64   pages;
> };

If you want to decrease robustness, this would need to be taken as part
of v23 review. It is too big design change to managed like this. I'm
not opionated here. This is just wrong order of doing things.

/Jarkko

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH for_v23 3/7] x86/sgx: Tweak ADD_PAGE ioctl to allow adding multiple pages
  2019-10-14 21:35     ` Jarkko Sakkinen
@ 2019-10-14 23:31       ` Sean Christopherson
  2019-10-16 10:17         ` Jarkko Sakkinen
  0 siblings, 1 reply; 18+ messages in thread
From: Sean Christopherson @ 2019-10-14 23:31 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx

On Tue, Oct 15, 2019 at 12:35:46AM +0300, Jarkko Sakkinen wrote:
> On Tue, Oct 15, 2019 at 12:32:55AM +0300, Jarkko Sakkinen wrote:
> > On Tue, Oct 08, 2019 at 09:42:37PM -0700, Sean Christopherson wrote:
> > > Add a nr_pages param to the ioctl for adding pages to the enclave so
> > > that userspace can batch multiple pages in a single syscall.  Update the
> > > offset, src and nr_pages params prior to returning to userspace so that
> > > the caller has sufficient information to analyze failures and can easily
> > > restart the ioctl when appropriate.
> > > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > Please provide a more robust API. Now you decrease the robustness.
> > 
> > E.g.
> > 
> > struct sgx_enclave_add_page_desc {
> >         __u64   offset;
> >         __u16   mrmask;
> >         __u8    reserved[6];
> > };
> > 
> > struct sgx_enclave_add_page {
> >         __u64   src;
> >         __u64   secinfo;
> >         __u64   nr_pages;
> >         __u64   pages;
> > };
> 
> If you want to decrease robustness, this would need to be taken as part
> of v23 review. It is too big design change to managed like this. I'm
> not opionated here. This is just wrong order of doing things.

I don't mind taking this to v23 review, but what do you mean by robustness
in this context?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH for_v23 3/7] x86/sgx: Tweak ADD_PAGE ioctl to allow adding multiple pages
  2019-10-14 23:31       ` Sean Christopherson
@ 2019-10-16 10:17         ` Jarkko Sakkinen
  2019-10-16 10:19           ` Jarkko Sakkinen
  0 siblings, 1 reply; 18+ messages in thread
From: Jarkko Sakkinen @ 2019-10-16 10:17 UTC (permalink / raw)
  To: Sean Christopherson, Jethro Beekman; +Cc: linux-sgx

On Mon, Oct 14, 2019 at 04:31:28PM -0700, Sean Christopherson wrote:
> I don't mind taking this to v23 review, but what do you mean by robustness
> in this context?

I think I kind of got this together API-wise:

#define SGX_ENCLAVE_ADD_PAGES_MEASURE 1

struct sgx_enclave_add_pages {
	__u64 src;
	__u64 offset;
	__u64 length;
	__u64 secinfo;
};

Length can be anything as long as low 8 bits are zero. The area
defined by offset and length is measured when
SGX_ENCLAVE_ADD_PAGES_MEASURE is set to 1.

I think this is the most sane API so far and does fulfill Jethro's
concerns why he originally wanted mrmask. I think this what most
users would find the most intuitive API.

Jethro, do you think you could live with this?

/Jarkko

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH for_v23 3/7] x86/sgx: Tweak ADD_PAGE ioctl to allow adding multiple pages
  2019-10-16 10:17         ` Jarkko Sakkinen
@ 2019-10-16 10:19           ` Jarkko Sakkinen
  2019-10-16 10:29             ` Jarkko Sakkinen
  0 siblings, 1 reply; 18+ messages in thread
From: Jarkko Sakkinen @ 2019-10-16 10:19 UTC (permalink / raw)
  To: Sean Christopherson, Jethro Beekman; +Cc: linux-sgx

On Wed, Oct 16, 2019 at 01:17:23PM +0300, Jarkko Sakkinen wrote:
> On Mon, Oct 14, 2019 at 04:31:28PM -0700, Sean Christopherson wrote:
> > I don't mind taking this to v23 review, but what do you mean by robustness
> > in this context?
> 
> I think I kind of got this together API-wise:
> 
> #define SGX_ENCLAVE_ADD_PAGES_MEASURE 1
> 
> struct sgx_enclave_add_pages {
> 	__u64 src;
> 	__u64 offset;
> 	__u64 length;
> 	__u64 secinfo;
> };
> 
> Length can be anything as long as low 8 bits are zero. The area
> defined by offset and length is measured when
> SGX_ENCLAVE_ADD_PAGES_MEASURE is set to 1.
> 
> I think this is the most sane API so far and does fulfill Jethro's
> concerns why he originally wanted mrmask. I think this what most
> users would find the most intuitive API.
> 
> Jethro, do you think you could live with this?

This just a version of Sean's API but with sane defaults for mrmask.

/Jarkko

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH for_v23 3/7] x86/sgx: Tweak ADD_PAGE ioctl to allow adding multiple pages
  2019-10-16 10:19           ` Jarkko Sakkinen
@ 2019-10-16 10:29             ` Jarkko Sakkinen
  2019-10-21 11:24               ` Jarkko Sakkinen
  0 siblings, 1 reply; 18+ messages in thread
From: Jarkko Sakkinen @ 2019-10-16 10:29 UTC (permalink / raw)
  To: Sean Christopherson, Jethro Beekman; +Cc: linux-sgx

On Wed, Oct 16, 2019 at 01:19:56PM +0300, Jarkko Sakkinen wrote:
> On Wed, Oct 16, 2019 at 01:17:23PM +0300, Jarkko Sakkinen wrote:
> > On Mon, Oct 14, 2019 at 04:31:28PM -0700, Sean Christopherson wrote:
> > > I don't mind taking this to v23 review, but what do you mean by robustness
> > > in this context?
> > 
> > I think I kind of got this together API-wise:
> > 
> > #define SGX_ENCLAVE_ADD_PAGES_MEASURE 1
> > 
> > struct sgx_enclave_add_pages {
> > 	__u64 src;
> > 	__u64 offset;
> > 	__u64 length;
> > 	__u64 secinfo;
> > };
> > 
> > Length can be anything as long as low 8 bits are zero. The area
> > defined by offset and length is measured when
> > SGX_ENCLAVE_ADD_PAGES_MEASURE is set to 1.
> > 
> > I think this is the most sane API so far and does fulfill Jethro's
> > concerns why he originally wanted mrmask. I think this what most
> > users would find the most intuitive API.
> > 
> > Jethro, do you think you could live with this?
> 
> This just a version of Sean's API but with sane defaults for mrmask.

Now that mrmask is rendered out the general idea of defining continuous
regions rather than scattered arrays of descriptors is superior. And it
is also obvious that a single page ioctl would be ugly glitch even if it
wouldn't cause harm.

/Jarkko

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH for_v23 3/7] x86/sgx: Tweak ADD_PAGE ioctl to allow adding multiple pages
  2019-10-16 10:29             ` Jarkko Sakkinen
@ 2019-10-21 11:24               ` Jarkko Sakkinen
  0 siblings, 0 replies; 18+ messages in thread
From: Jarkko Sakkinen @ 2019-10-21 11:24 UTC (permalink / raw)
  To: Sean Christopherson, Jethro Beekman; +Cc: linux-sgx

On Wed, Oct 16, 2019 at 01:29:12PM +0300, Jarkko Sakkinen wrote:
> On Wed, Oct 16, 2019 at 01:19:56PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Oct 16, 2019 at 01:17:23PM +0300, Jarkko Sakkinen wrote:
> > > On Mon, Oct 14, 2019 at 04:31:28PM -0700, Sean Christopherson wrote:
> > > > I don't mind taking this to v23 review, but what do you mean by robustness
> > > > in this context?
> > > 
> > > I think I kind of got this together API-wise:
> > > 
> > > #define SGX_ENCLAVE_ADD_PAGES_MEASURE 1
> > > 
> > > struct sgx_enclave_add_pages {
> > > 	__u64 src;
> > > 	__u64 offset;
> > > 	__u64 length;
> > > 	__u64 secinfo;
> > > };

Offset would be split as 48:8:8 aka PAGE_INDEX:MRSTART:FLAGS
Length would be split as 48:8:8 aka NR_PAGES:MREND:0

Forgot from my earlier description that measurement starting
point would better to be also after the page start but with
this bit presentation everything can be cleanly described.

/Jarkko

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2019-10-21 11:25 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-09  4:42 [PATCH for_v23 0/7] x86/sgx: Improve add pages ioctl Sean Christopherson
2019-10-09  4:42 ` [PATCH for_v23 1/7] x86/sgx: Modify ADD_PAGE ioctl to take offset instead of full address Sean Christopherson
2019-10-09  4:42 ` [PATCH for_v23 2/7] selftests/x86/sgx: Update test to account for ADD_PAGE change Sean Christopherson
2019-10-09  4:42 ` [PATCH for_v23 3/7] x86/sgx: Tweak ADD_PAGE ioctl to allow adding multiple pages Sean Christopherson
2019-10-14 21:32   ` Jarkko Sakkinen
2019-10-14 21:35     ` Jarkko Sakkinen
2019-10-14 23:31       ` Sean Christopherson
2019-10-16 10:17         ` Jarkko Sakkinen
2019-10-16 10:19           ` Jarkko Sakkinen
2019-10-16 10:29             ` Jarkko Sakkinen
2019-10-21 11:24               ` Jarkko Sakkinen
2019-10-09  4:42 ` [PATCH for_v23 4/7] selftests/x86/sgx: Update enclave build flow to do multi-page add Sean Christopherson
2019-10-09  4:42 ` [PATCH for_v23 5/7] x86/sgx: Add a flag to ADD_PAGES to allow replicating the source page Sean Christopherson
2019-10-09  4:42 ` [PATCH for_v23 6/7] selftests/x86/sgx: Update selftest to account for ADD_PAGES flag Sean Christopherson
2019-10-09  4:42 ` [PATCH for_v23 7/7] selftests/x86/sgx: Add test coverage for reclaim and replicate Sean Christopherson
2019-10-10  3:28 ` [PATCH for_v23 0/7] x86/sgx: Improve add pages ioctl Haitao Huang
2019-10-11 14:37   ` Sean Christopherson
2019-10-13 15:15     ` Dr. Greg

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).