Linux-Sgx Archive on lore.kernel.org
 help / color / Atom feed
From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: linux-sgx@vger.kernel.org, Dave Hansen <dave.hansen@intel.com>,
	Cedric Xing <cedric.xing@intel.com>,
	Andy Lutomirski <luto@kernel.org>,
	Jethro Beekman <jethro@fortanix.com>,
	"Dr . Greg Wettstein" <greg@enjellic.com>
Subject: [PATCH 3/7] x86/sgx: Let ioctl helpers do copy to/from user
Date: Wed,  5 Jun 2019 12:48:41 -0700
Message-ID: <20190605194845.926-4-sean.j.christopherson@intel.com> (raw)
In-Reply-To: <20190605194845.926-1-sean.j.christopherson@intel.com>

A future patch will extend SGX_IOC_ENCLAVE_ADD_PAGE to allow userspace
to add multiple pages in a single ioctl.  Because the number of pages
could be quite high, it's possible the ioctl could be interrupted, in
which case the driver will need to "return" the number of pages that
have been queued, e.g. so that userspace can restart the ioctl in the
event that the signal wasn't fatal.

In short, SGX_IOC_ENCLAVE_ADD_PAGE will become a conditional _IOWR
and will only do copy_to_user() on a non-zero return value.  Rather than
implement funky logic, pass the raw userspace argument to the helpers
so that they may do copy_{to,from}_user() at will.  This kills
pre-copying the data, but the value added by pre-copy is tenuous, e.g.
it consolidates two lines of code but requires use of a fixed size
buffer as well as an indirect handler and thus retpoline.  And the
pre-copy may lead to reader confusion due to @arg being a user pointer
in some flows and a kernel pointer in others.  Lastly, in the event that
a pure _IOR ioctl is added, there won't be a need to update the existing
code (to skip copy_from_user()).

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 73 ++++++++++++--------------
 1 file changed, 34 insertions(+), 39 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index a0742d18aa36..d17c60dca114 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -383,7 +383,7 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 /**
  * sgx_ioc_enclave_create - handler for %SGX_IOC_ENCLAVE_CREATE
  * @filep:	open file to /dev/sgx
- * @arg:	pointer to an &sgx_enclave_create instance
+ * @arg:	userspace pointer to a struct sgx_enclave_create instance
  *
  * Allocate kernel data structures for a new enclave and execute ECREATE after
  * verifying the correctness of the provided SECS.
@@ -396,25 +396,27 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
  *   0 on success,
  *   -errno otherwise
  */
-static long sgx_ioc_enclave_create(struct file *filep, unsigned long arg)
+static long sgx_ioc_enclave_create(struct file *filep, void __user *arg)
 {
-	struct sgx_enclave_create *createp = (struct sgx_enclave_create *)arg;
 	struct sgx_encl *encl = filep->private_data;
+	struct sgx_enclave_create ecreate;
 	struct page *secs_page;
 	struct sgx_secs *secs;
 	int ret;
 
+	if (copy_from_user(&ecreate, arg, sizeof(ecreate)))
+		return -EFAULT;
+
 	secs_page = alloc_page(GFP_HIGHUSER);
 	if (!secs_page)
 		return -ENOMEM;
 
 	secs = kmap(secs_page);
-	if (copy_from_user(secs, (void __user *)createp->src, sizeof(*secs))) {
+	if (copy_from_user(secs, (void __user *)ecreate.src, sizeof(*secs))) {
 		ret = -EFAULT;
 		goto out;
 	}
 
-
 	ret = sgx_encl_create(encl, secs);
 
 out:
@@ -577,7 +579,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
  * sgx_ioc_enclave_add_page - handler for %SGX_IOC_ENCLAVE_ADD_PAGE
  *
  * @filep:	open file to /dev/sgx
- * @arg:	pointer to an &sgx_enclave_add_page instance
+ * @arg:	userspace pointer to a struct sgx_enclave_add_page instance
  *
  * Add a page to an uninitialized enclave (EADD), and optionally extend the
  * enclave's measurement with the contents of the page (EEXTEND).  EADD and
@@ -590,16 +592,19 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
  *   0 on success,
  *   -errno otherwise
  */
-static long sgx_ioc_enclave_add_page(struct file *filep, unsigned long arg)
+static long sgx_ioc_enclave_add_page(struct file *filep, void __user *arg)
 {
-	struct sgx_enclave_add_page *addp = (void *)arg;
 	struct sgx_encl *encl = filep->private_data;
+	struct sgx_enclave_add_page addp;
 	struct sgx_secinfo secinfo;
 	struct page *data_page;
 	void *data;
 	int ret;
 
-	if (copy_from_user(&secinfo, (void __user *)addp->secinfo,
+	if (copy_from_user(&addp, arg, sizeof(addp)))
+		return -EFAULT;
+
+	if (copy_from_user(&secinfo, (void __user *)addp.secinfo,
 			   sizeof(secinfo)))
 		return -EFAULT;
 
@@ -609,12 +614,12 @@ static long sgx_ioc_enclave_add_page(struct file *filep, unsigned long arg)
 
 	data = kmap(data_page);
 
-	if (copy_from_user((void *)data, (void __user *)addp->src, PAGE_SIZE)) {
+	if (copy_from_user((void *)data, (void __user *)addp.src, PAGE_SIZE)) {
 		ret = -EFAULT;
 		goto out;
 	}
 
-	ret = sgx_encl_add_page(encl, addp->addr, data, &secinfo, addp->mrmask);
+	ret = sgx_encl_add_page(encl, addp.addr, data, &secinfo, addp.mrmask);
 	if (ret)
 		goto out;
 
@@ -717,7 +722,7 @@ static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct,
  * sgx_ioc_enclave_init - handler for %SGX_IOC_ENCLAVE_INIT
  *
  * @filep:	open file to /dev/sgx
- * @arg:	pointer to an &sgx_enclave_init instance
+ * @arg:	userspace pointer to a struct sgx_enclave_init instance
  *
  * Flush any outstanding enqueued EADD operations and perform EINIT.  The
  * Launch Enclave Public Key Hash MSRs are rewritten as necessary to match
@@ -728,15 +733,18 @@ static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct,
  *   SGX error code on EINIT failure,
  *   -errno otherwise
  */
-static long sgx_ioc_enclave_init(struct file *filep, unsigned long arg)
+static long sgx_ioc_enclave_init(struct file *filep, void __user *arg)
 {
-	struct sgx_enclave_init *initp = (struct sgx_enclave_init *)arg;
 	struct sgx_encl *encl = filep->private_data;
 	struct sgx_einittoken *einittoken;
 	struct sgx_sigstruct *sigstruct;
+	struct sgx_enclave_init einit;
 	struct page *initp_page;
 	int ret;
 
+	if (copy_from_user(&einit, arg, sizeof(einit)))
+		return -EFAULT;
+
 	initp_page = alloc_page(GFP_HIGHUSER);
 	if (!initp_page)
 		return -ENOMEM;
@@ -746,13 +754,12 @@ static long sgx_ioc_enclave_init(struct file *filep, unsigned long arg)
 		((unsigned long)sigstruct + PAGE_SIZE / 2);
 	memset(einittoken, 0, sizeof(*einittoken));
 
-	if (copy_from_user(sigstruct, (void __user *)initp->sigstruct,
+	if (copy_from_user(sigstruct, (void __user *)einit.sigstruct,
 			   sizeof(*sigstruct))) {
 		ret = -EFAULT;
 		goto out;
 	}
 
-
 	ret = sgx_encl_init(encl, sigstruct, einittoken);
 
 out:
@@ -764,7 +771,7 @@ static long sgx_ioc_enclave_init(struct file *filep, unsigned long arg)
 /**
  * sgx_ioc_enclave_set_attribute - handler for %SGX_IOC_ENCLAVE_SET_ATTRIBUTE
  * @filep:	open file to /dev/sgx
- * @arg:	pointer to a struct sgx_enclave_set_attribute instance
+ * @arg:	userspace pointer to a struct sgx_enclave_set_attribute instance
  *
  * Mark the enclave as being allowed to access a restricted attribute bit.
  * The requested attribute is specified via the attribute_fd field in the
@@ -779,14 +786,17 @@ static long sgx_ioc_enclave_init(struct file *filep, unsigned long arg)
  *
  * Return: 0 on success, -errno otherwise
  */
-static long sgx_ioc_enclave_set_attribute(struct file *filep, unsigned long arg)
+static long sgx_ioc_enclave_set_attribute(struct file *filep, void __user *arg)
 {
-	struct sgx_enclave_set_attribute *params = (void *)arg;
 	struct sgx_encl *encl = filep->private_data;
+	struct sgx_enclave_set_attribute params;
 	struct file *attribute_file;
 	int ret;
 
-	attribute_file = fget(params->attribute_fd);
+	if (copy_from_user(&params, arg, sizeof(params)))
+		return -EFAULT;
+
+	attribute_file = fget(params.attribute_fd);
 	if (!attribute_file->f_op)
 		return -EINVAL;
 
@@ -802,33 +812,18 @@ static long sgx_ioc_enclave_set_attribute(struct file *filep, unsigned long arg)
 	return ret;
 }
 
-typedef long (*sgx_ioc_t)(struct file *filep, unsigned long arg);
-
 long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 {
-	char data[256];
-	sgx_ioc_t handler = NULL;
-	long ret;
-
 	switch (cmd) {
 	case SGX_IOC_ENCLAVE_CREATE:
-		handler = sgx_ioc_enclave_create;
-		break;
+		return sgx_ioc_enclave_create(filep, (void __user *)arg);
 	case SGX_IOC_ENCLAVE_ADD_PAGE:
-		handler = sgx_ioc_enclave_add_page;
-		break;
+		return sgx_ioc_enclave_add_page(filep, (void __user *)arg);
 	case SGX_IOC_ENCLAVE_INIT:
-		handler = sgx_ioc_enclave_init;
-		break;
+		return sgx_ioc_enclave_init(filep, (void __user *)arg);
 	case SGX_IOC_ENCLAVE_SET_ATTRIBUTE:
-		handler = sgx_ioc_enclave_set_attribute;
-		break;
+		return sgx_ioc_enclave_set_attribute(filep, (void __user *)arg);
 	default:
 		return -ENOIOCTLCMD;
 	}
-
-	if (copy_from_user(data, (void __user *)arg, _IOC_SIZE(cmd)))
-		return -EFAULT;
-
-	return handler(filep, (unsigned long)((void *)data));
 }
-- 
2.21.0


  parent reply index

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-05 19:48 [PATCH 0/7] x86/sgx: Clean up and enhance add pages ioctl Sean Christopherson
2019-06-05 19:48 ` [PATCH 1/7] x86/sgx: Remove dead code to handle non-existent IOR ioctl Sean Christopherson
2019-06-05 19:48 ` [PATCH 2/7] x86/sgx: Remove unnecessary @cmd parameter from ioctl helpers Sean Christopherson
2019-06-05 19:48 ` Sean Christopherson [this message]
2019-06-05 19:48 ` [PATCH 4/7] x86/sgx: Allow userspace to add multiple pages in single ioctl() Sean Christopherson
2019-06-06 15:47   ` Jarkko Sakkinen
2019-06-13  0:43   ` Jethro Beekman
2019-06-13 16:51     ` Sean Christopherson
2019-06-13 19:05       ` Andy Lutomirski
2019-06-13 19:15         ` Sean Christopherson
2019-06-13 19:45       ` Xing, Cedric
2019-06-05 19:48 ` [PATCH 5/7] x86/sgx: Add flag to zero added region instead of copying from source Sean Christopherson
2019-06-06 17:20   ` Andy Lutomirski
2019-06-06 17:32     ` Sean Christopherson
2019-06-07 19:32       ` Andy Lutomirski
2019-06-10 17:06         ` Jarkko Sakkinen
2019-06-10 18:09         ` Xing, Cedric
2019-06-10 18:41           ` Sean Christopherson
2019-06-10 18:53         ` Sean Christopherson
2019-06-13  0:38           ` Jethro Beekman
2019-06-13 13:46             ` Sean Christopherson
2019-06-13 16:16               ` Andy Lutomirski
2019-06-13 16:54                 ` Sean Christopherson
2019-06-05 19:48 ` [PATCH 6/7] x86/sgx: Use the actual zero page as the source when adding zero pages Sean Christopherson
2019-06-05 19:48 ` [PATCH 7/7] x86/sgx: Add a reserved field to sgx_enclave_add_region to drop 'packed' Sean Christopherson
2019-06-05 19:59   ` Dave Hansen
2019-06-05 20:00     ` Andy Lutomirski
2019-06-12 15:14   ` Jarkko Sakkinen
2019-06-12 15:23     ` Sean Christopherson
2019-06-13  0:44       ` Jethro Beekman
2019-06-13 15:38       ` Jarkko Sakkinen
2019-06-12 15:16 ` [PATCH 0/7] x86/sgx: Clean up and enhance add pages ioctl Jarkko Sakkinen
2019-06-12 18:14   ` Jarkko Sakkinen

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190605194845.926-4-sean.j.christopherson@intel.com \
    --to=sean.j.christopherson@intel.com \
    --cc=cedric.xing@intel.com \
    --cc=dave.hansen@intel.com \
    --cc=greg@enjellic.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=jethro@fortanix.com \
    --cc=linux-sgx@vger.kernel.org \
    --cc=luto@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Sgx Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-sgx/0 linux-sgx/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-sgx linux-sgx/ https://lore.kernel.org/linux-sgx \
		linux-sgx@vger.kernel.org linux-sgx@archiver.kernel.org
	public-inbox-index linux-sgx


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-sgx


AGPL code for this site: git clone https://public-inbox.org/ public-inbox