[for,v24,v2,4/4] x86/sgx: add @count to &sgx_enclave_add_pages
diff mbox series

Message ID 20191105112056.21452-4-jarkko.sakkinen@linux.intel.com
State New
Headers show
Series
  • [for,v24,v2,1/4] x86/sgx: Destroy enclave if EADD fails
Related show

Commit Message

Jarkko Sakkinen Nov. 5, 2019, 11:20 a.m. UTC
Add @count write the number of bytes added as there is not any good reason
to overwrite input parameters. Also, three parameters are unnecessarily
overwritten as the amount of change is the same for each of them.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/include/uapi/asm/sgx.h |  2 ++
 arch/x86/kernel/cpu/sgx/ioctl.c | 17 ++++++-----------
 2 files changed, 8 insertions(+), 11 deletions(-)

Comments

Sean Christopherson Nov. 5, 2019, 10:52 p.m. UTC | #1
On Tue, Nov 05, 2019 at 01:20:56PM +0200, Jarkko Sakkinen wrote:
> Add @count write the number of bytes added as there is not any good reason
> to overwrite input parameters.

I disagree, overwriting the params means userspace doesn't need to adjust
the values to restart the ioctl().  Ditto for printing out the failing
address if the ioctl() fails.

> Also, three parameters are unnecessarily
> overwritten as the amount of change is the same for each of them.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  arch/x86/include/uapi/asm/sgx.h |  2 ++
>  arch/x86/kernel/cpu/sgx/ioctl.c | 17 ++++++-----------
>  2 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index 88644b6ad849..e196cfd44b70 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -45,6 +45,7 @@ struct sgx_enclave_create  {
>   * @length:	length of the data (multiple of the page size)
>   * @secinfo:	address for the SECINFO data
>   * @flags:	page control flags
> + * @count:	number of bytes added (multiple of the page size)
>   */
>  struct sgx_enclave_add_pages {
>  	__u64	src;
> @@ -52,6 +53,7 @@ struct sgx_enclave_add_pages {
>  	__u64	length;
>  	__u64	secinfo;
>  	__u64	flags;
> +	__u64	count;
>  };
>  
>  /**
> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> index deca49bd4f58..e8697d145dfb 100644
> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -491,11 +491,6 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
>   * 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->length 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,
>   *   -EACCES if an executable source page is located in a noexec partition,
> @@ -506,6 +501,7 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
>  {
>  	struct sgx_enclave_add_pages addp;
>  	struct sgx_secinfo secinfo;
> +	unsigned long c;
>  	int ret;
>  
>  	if (!(atomic_read(&encl->flags) & SGX_ENCL_CREATED))
> @@ -534,7 +530,7 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
>  	if (sgx_validate_secinfo(&secinfo))
>  		return -EINVAL;
>  
> -	for ( ; addp.length > 0; addp.length -= PAGE_SIZE) {
> +	for (c = 0 ; c < addp.length; c += PAGE_SIZE) {
>  		if (signal_pending(current)) {
>  			ret = -ERESTARTSYS;
>  			break;
> @@ -543,15 +539,14 @@ 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, addp.src, addp.offset,
> -					addp.length, &secinfo, addp.flags);
> +		ret = sgx_encl_add_page(encl, addp.src + c, addp.offset + c,
> +					addp.length - c, &secinfo, addp.flags);
>  		if (ret)
>  			break;
> -
> -		addp.offset += PAGE_SIZE;
> -		addp.src += PAGE_SIZE;
>  	}
>  
> +	addp.count = c;
> +
>  	if (copy_to_user(arg, &addp, sizeof(addp)))
>  		return -EFAULT;
>  
> -- 
> 2.20.1
>
Jarkko Sakkinen Nov. 6, 2019, 11:20 p.m. UTC | #2
On Tue, Nov 05, 2019 at 02:52:23PM -0800, Sean Christopherson wrote:
> On Tue, Nov 05, 2019 at 01:20:56PM +0200, Jarkko Sakkinen wrote:
> > Add @count write the number of bytes added as there is not any good reason
> > to overwrite input parameters.
> 
> I disagree, overwriting the params means userspace doesn't need to adjust
> the values to restart the ioctl().  Ditto for printing out the failing
> address if the ioctl() fails.

There is three redundant updates. At least only @length must be
updated in order to remove this glitch.

As far as overwriting goes, it should be only done when there is
requiring to do that.

/Jarkko
Jarkko Sakkinen Nov. 8, 2019, 8:13 a.m. UTC | #3
On Thu, Nov 07, 2019 at 01:20:30AM +0200, Jarkko Sakkinen wrote:
> On Tue, Nov 05, 2019 at 02:52:23PM -0800, Sean Christopherson wrote:
> > On Tue, Nov 05, 2019 at 01:20:56PM +0200, Jarkko Sakkinen wrote:
> > > Add @count write the number of bytes added as there is not any good reason
> > > to overwrite input parameters.
> > 
> > I disagree, overwriting the params means userspace doesn't need to adjust
> > the values to restart the ioctl().  Ditto for printing out the failing
> > address if the ioctl() fails.
> 
> There is three redundant updates. At least only @length must be
> updated in order to remove this glitch.
> 
> As far as overwriting goes, it should be only done when there is
> requiring to do that.

What is obvious is that the current behaviour is wrong. You have
a *single value* to return and you encode the *same value* with
*three encodings*:

1. offset + count
2. length - count
3. src + count

And ironically none of the encodings give you the count of bytes
processed in unpacked form. It is something that must be readily
available as practically all common syscalls that can partially process
the input give that. There is a long history of that pattern and no
history at all with this weird pack of encodings.

/Jarkko

Patch
diff mbox series

diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 88644b6ad849..e196cfd44b70 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -45,6 +45,7 @@  struct sgx_enclave_create  {
  * @length:	length of the data (multiple of the page size)
  * @secinfo:	address for the SECINFO data
  * @flags:	page control flags
+ * @count:	number of bytes added (multiple of the page size)
  */
 struct sgx_enclave_add_pages {
 	__u64	src;
@@ -52,6 +53,7 @@  struct sgx_enclave_add_pages {
 	__u64	length;
 	__u64	secinfo;
 	__u64	flags;
+	__u64	count;
 };
 
 /**
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index deca49bd4f58..e8697d145dfb 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -491,11 +491,6 @@  static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
  * 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->length 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,
  *   -EACCES if an executable source page is located in a noexec partition,
@@ -506,6 +501,7 @@  static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
 {
 	struct sgx_enclave_add_pages addp;
 	struct sgx_secinfo secinfo;
+	unsigned long c;
 	int ret;
 
 	if (!(atomic_read(&encl->flags) & SGX_ENCL_CREATED))
@@ -534,7 +530,7 @@  static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
 	if (sgx_validate_secinfo(&secinfo))
 		return -EINVAL;
 
-	for ( ; addp.length > 0; addp.length -= PAGE_SIZE) {
+	for (c = 0 ; c < addp.length; c += PAGE_SIZE) {
 		if (signal_pending(current)) {
 			ret = -ERESTARTSYS;
 			break;
@@ -543,15 +539,14 @@  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, addp.src, addp.offset,
-					addp.length, &secinfo, addp.flags);
+		ret = sgx_encl_add_page(encl, addp.src + c, addp.offset + c,
+					addp.length - c, &secinfo, addp.flags);
 		if (ret)
 			break;
-
-		addp.offset += PAGE_SIZE;
-		addp.src += PAGE_SIZE;
 	}
 
+	addp.count = c;
+
 	if (copy_to_user(arg, &addp, sizeof(addp)))
 		return -EFAULT;