[PATCH] x86/sgx: Roof the number of pages process in SGX_IOC_ENCLAVE_ADD_PAGES

[PATCH] x86/sgx: Roof the number of pages process in SGX_IOC_ENCLAVE_ADD_PAGES

[Jarkko Sakkinen]

Remove 'count' from struct sgx_enclave_add_pages and return number of
pages processed as a positive return as there is no reasonable use for
-EINTR for the caller. Then, roof the processing with a fixed constant
and break out the loop also when this value is surpassed.

Link: https://lore.kernel.org/linux-sgx/20200626153400.GE27151@zn.tnic/
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: Borislav Petkov <bp@suse.de>
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 | 10 ++++------
 arch/x86/kernel/cpu/sgx/sgx.h   |  1 +
 3 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index b3d5ccf5b976..1564d7f88597 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -45,7 +45,6 @@ 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;
@@ -53,7 +52,6 @@ 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 6c3c6177b690..0ae00fa9b589 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -490,7 +490,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
  * caller.
  *
  * Return:
- *   0 on success,
+ *   length of the data processed on success,
  *   -EACCES if an executable source page is located in a noexec partition,
  *   -EIO if either ENCLS[EADD] or ENCLS[EEXTEND] fails
  *   -errno otherwise
@@ -530,8 +530,8 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
 		return -EINVAL;
 
 	for (c = 0 ; c < addp.length; c += PAGE_SIZE) {
-		if (signal_pending(current)) {
-			ret = -EINTR;
+		if (c == SGX_MAX_ADD_PAGES_LENGTH || signal_pending(current)) {
+			ret = c;
 			break;
 		}
 
@@ -544,12 +544,10 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
 			break;
 	}
 
-	addp.count = c;
-
 	if (copy_to_user(arg, &addp, sizeof(addp)))
 		return -EFAULT;
 
-	return ret;
+	return c;
 }
 
 static int __sgx_get_key_hash(struct crypto_shash *tfm, const void *modulus,
diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index 1a6ca5f734e5..94b29f378ea2 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -37,6 +37,7 @@ struct sgx_epc_section {
 #define SGX_EPC_SECTION_MASK		GENMASK(7, 0)
 #define SGX_MAX_EPC_SECTIONS		(SGX_EPC_SECTION_MASK + 1)
 #define SGX_EPC_PAGE_RECLAIMABLE	BIT(8)
+#define SGX_MAX_ADD_PAGES_LENGTH	0x100000
 #define SGX_NR_TO_SCAN			16
 #define SGX_NR_LOW_PAGES		32
 #define SGX_NR_HIGH_PAGES		64
-- 
2.25.1

Re: [PATCH] x86/sgx: Roof the number of pages process in SGX_IOC_ENCLAVE_ADD_PAGES

[Sean Christopherson]

On Tue, Sep 08, 2020 at 10:00:42PM +0300, Jarkko Sakkinen wrote:
> Remove 'count' from struct sgx_enclave_add_pages and return number of
> pages processed as a positive return as there is no reasonable use for
> -EINTR for the caller. Then, roof the processing with a fixed constant
> and break out the loop also when this value is surpassed.
> 
> Link: https://lore.kernel.org/linux-sgx/20200626153400.GE27151@zn.tnic/
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Cc: Borislav Petkov <bp@suse.de>
> 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 | 10 ++++------
>  arch/x86/kernel/cpu/sgx/sgx.h   |  1 +
>  3 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index b3d5ccf5b976..1564d7f88597 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -45,7 +45,6 @@ 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;
> @@ -53,7 +52,6 @@ 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 6c3c6177b690..0ae00fa9b589 100644
> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -490,7 +490,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
>   * caller.
>   *
>   * Return:
> - *   0 on success,
> + *   length of the data processed on success,
>   *   -EACCES if an executable source page is located in a noexec partition,
>   *   -EIO if either ENCLS[EADD] or ENCLS[EEXTEND] fails
>   *   -errno otherwise
> @@ -530,8 +530,8 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
>  		return -EINVAL;
>  
>  	for (c = 0 ; c < addp.length; c += PAGE_SIZE) {
> -		if (signal_pending(current)) {
> -			ret = -EINTR;
> +		if (c == SGX_MAX_ADD_PAGES_LENGTH || signal_pending(current)) {
> +			ret = c;

I don't have an opinion on returning count vs. EINTR, but I don't see the
point in arbitrarily capping the number of pages that can be added in a
single ioctl().  It doesn't provide any real protection, e.g. userspace
can simply restart the ioctl() with updated offsets and continue spamming
EADDs.  We are relying on other limits, e.g. memcg, rlimits, etc... to
reign in malicious/broken userspace.

There is nothing inherently dangerous about spending time in the kernel so
long as appropriate checks are made, e.g. for a pending signel and resched.
If we're missing checks, adding an arbitrary limit won't fix the underlying
problem, at least not in a deterministic way.

If we really want a limit of some form, adding a knob to control the max
size of an enclave seems like the way to go.  But even that is of dubious
value as I'd rather rely on existing limits for virtual and physical memory,
and add a proper EPC cgroup to account and limit EPC memory.

>  			break;
>  		}
>  
> @@ -544,12 +544,10 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
>  			break;
>  	}
>  
> -	addp.count = c;
> -
>  	if (copy_to_user(arg, &addp, sizeof(addp)))
>  		return -EFAULT;
>  
> -	return ret;
> +	return c;
>  }
>  
>  static int __sgx_get_key_hash(struct crypto_shash *tfm, const void *modulus,
> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> index 1a6ca5f734e5..94b29f378ea2 100644
> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> @@ -37,6 +37,7 @@ struct sgx_epc_section {
>  #define SGX_EPC_SECTION_MASK		GENMASK(7, 0)
>  #define SGX_MAX_EPC_SECTIONS		(SGX_EPC_SECTION_MASK + 1)
>  #define SGX_EPC_PAGE_RECLAIMABLE	BIT(8)
> +#define SGX_MAX_ADD_PAGES_LENGTH	0x100000
>  #define SGX_NR_TO_SCAN			16
>  #define SGX_NR_LOW_PAGES		32
>  #define SGX_NR_HIGH_PAGES		64
> -- 
> 2.25.1
> 

Re: [PATCH] x86/sgx: Roof the number of pages process in SGX_IOC_ENCLAVE_ADD_PAGES

[Darren Kenny]

On Tuesday, 2020-09-08 at 22:00:42 +03, Jarkko Sakkinen wrote:
> Remove 'count' from struct sgx_enclave_add_pages and return number of
> pages processed as a positive return as there is no reasonable use for
> -EINTR for the caller. Then, roof the processing with a fixed constant
> and break out the loop also when this value is surpassed.
>
> Link: https://lore.kernel.org/linux-sgx/20200626153400.GE27151@zn.tnic/
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Cc: Borislav Petkov <bp@suse.de>
> 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 | 10 ++++------
>  arch/x86/kernel/cpu/sgx/sgx.h   |  1 +
>  3 files changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index b3d5ccf5b976..1564d7f88597 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -45,7 +45,6 @@ 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;
> @@ -53,7 +52,6 @@ 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 6c3c6177b690..0ae00fa9b589 100644
> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -490,7 +490,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
>   * caller.
>   *
>   * Return:
> - *   0 on success,
> + *   length of the data processed on success,
>   *   -EACCES if an executable source page is located in a noexec partition,
>   *   -EIO if either ENCLS[EADD] or ENCLS[EEXTEND] fails
>   *   -errno otherwise
> @@ -530,8 +530,8 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
>  		return -EINVAL;
>  
>  	for (c = 0 ; c < addp.length; c += PAGE_SIZE) {
> -		if (signal_pending(current)) {
> -			ret = -EINTR;
> +		if (c == SGX_MAX_ADD_PAGES_LENGTH || signal_pending(current)) {

I know that SGX_MAX_ADD_PAGES_LENGTH may be a multiple of PAGE_SIZE, but
to be sure that nothing breaks here in the future, I think it would be
better to use '>=' as the comparison rather than just '=='.

Also, the indentation seems off here w.r.t. using TABs when the break
below is using spaces - would be good to have them the same at least.

Thanks,

Darren.

> +			ret = c;
>  			break;
>  		}
>  
> @@ -544,12 +544,10 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
>  			break;
>  	}
>  
> -	addp.count = c;
> -
>  	if (copy_to_user(arg, &addp, sizeof(addp)))
>  		return -EFAULT;
>  
> -	return ret;
> +	return c;
>  }
>  
>  static int __sgx_get_key_hash(struct crypto_shash *tfm, const void *modulus,
> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> index 1a6ca5f734e5..94b29f378ea2 100644
> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> @@ -37,6 +37,7 @@ struct sgx_epc_section {
>  #define SGX_EPC_SECTION_MASK		GENMASK(7, 0)
>  #define SGX_MAX_EPC_SECTIONS		(SGX_EPC_SECTION_MASK + 1)
>  #define SGX_EPC_PAGE_RECLAIMABLE	BIT(8)
> +#define SGX_MAX_ADD_PAGES_LENGTH	0x100000
>  #define SGX_NR_TO_SCAN			16
>  #define SGX_NR_LOW_PAGES		32
>  #define SGX_NR_HIGH_PAGES		64
> -- 
> 2.25.1

Re: [PATCH] x86/sgx: Roof the number of pages process in SGX_IOC_ENCLAVE_ADD_PAGES

[Jarkko Sakkinen]

On Wed, Sep 09, 2020 at 10:55:58AM +0100, Darren Kenny wrote:
> On Tuesday, 2020-09-08 at 22:00:42 +03, Jarkko Sakkinen wrote:
> > Remove 'count' from struct sgx_enclave_add_pages and return number of
> > pages processed as a positive return as there is no reasonable use for
> > -EINTR for the caller. Then, roof the processing with a fixed constant
> > and break out the loop also when this value is surpassed.
> >
> > Link: https://lore.kernel.org/linux-sgx/20200626153400.GE27151@zn.tnic/
> > Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> > Cc: Borislav Petkov <bp@suse.de>
> > 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 | 10 ++++------
> >  arch/x86/kernel/cpu/sgx/sgx.h   |  1 +
> >  3 files changed, 5 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> > index b3d5ccf5b976..1564d7f88597 100644
> > --- a/arch/x86/include/uapi/asm/sgx.h
> > +++ b/arch/x86/include/uapi/asm/sgx.h
> > @@ -45,7 +45,6 @@ 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;
> > @@ -53,7 +52,6 @@ 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 6c3c6177b690..0ae00fa9b589 100644
> > --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> > @@ -490,7 +490,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
> >   * caller.
> >   *
> >   * Return:
> > - *   0 on success,
> > + *   length of the data processed on success,
> >   *   -EACCES if an executable source page is located in a noexec partition,
> >   *   -EIO if either ENCLS[EADD] or ENCLS[EEXTEND] fails
> >   *   -errno otherwise
> > @@ -530,8 +530,8 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
> >  		return -EINVAL;
> >  
> >  	for (c = 0 ; c < addp.length; c += PAGE_SIZE) {
> > -		if (signal_pending(current)) {
> > -			ret = -EINTR;
> > +		if (c == SGX_MAX_ADD_PAGES_LENGTH || signal_pending(current)) {
> 
> I know that SGX_MAX_ADD_PAGES_LENGTH may be a multiple of PAGE_SIZE, but
> to be sure that nothing breaks here in the future, I think it would be
> better to use '>=' as the comparison rather than just '=='.


I'm not forseeing this. We will change the comparison or remove it if
the constant is no longer required.

> 
> Also, the indentation seems off here w.r.t. using TABs when the break
> below is using spaces - would be good to have them the same at least.

Ugh, that must be my mistake. I'll check these before squashing this.

Thank you.



/Jarkko

Re: [PATCH] x86/sgx: Roof the number of pages process in SGX_IOC_ENCLAVE_ADD_PAGES

[Sean Christopherson]

On Fri, Sep 11, 2020 at 02:43:15PM +0300, Jarkko Sakkinen wrote:
> On Tue, Sep 08, 2020 at 10:30:33PM -0700, Sean Christopherson wrote:
> > >  	for (c = 0 ; c < addp.length; c += PAGE_SIZE) {
> > > -		if (signal_pending(current)) {
> > > -			ret = -EINTR;
> > > +		if (c == SGX_MAX_ADD_PAGES_LENGTH || signal_pending(current)) {
> > > +			ret = c;
> > 
> > I don't have an opinion on returning count vs. EINTR, but I don't see the
> > point in arbitrarily capping the number of pages that can be added in a
> > single ioctl().  It doesn't provide any real protection, e.g. userspace
> > can simply restart the ioctl() with updated offsets and continue spamming
> > EADDs.  We are relying on other limits, e.g. memcg, rlimits, etc... to
> > reign in malicious/broken userspace.
> > 
> > There is nothing inherently dangerous about spending time in the kernel so
> > long as appropriate checks are made, e.g. for a pending signel and resched.
> > If we're missing checks, adding an arbitrary limit won't fix the underlying
> > problem, at least not in a deterministic way.
> > 
> > If we really want a limit of some form, adding a knob to control the max
> > size of an enclave seems like the way to go.  But even that is of dubious
> > value as I'd rather rely on existing limits for virtual and physical memory,
> > and add a proper EPC cgroup to account and limit EPC memory.
> 
> It is better to have a contract in the API that the number of processed
> pages can be less than given, not unlike in syscalls such as write().

That can be handled by a comment, no?  If we want to "enforce" the behavior,
I'd rather bail out of the loop after a random number of pages than have a
completely arbitrary limit.  The arbitrary limit will create a contract of
its own and may lead to weird guest implementations.

Re: [PATCH] x86/sgx: Roof the number of pages process in SGX_IOC_ENCLAVE_ADD_PAGES

[Haitao Huang]

nOn Fri, 11 Sep 2020 10:51:27 -0500, Sean Christopherson  
<sean.j.christopherson@intel.com> wrote:

> On Fri, Sep 11, 2020 at 02:43:15PM +0300, Jarkko Sakkinen wrote:
>> On Tue, Sep 08, 2020 at 10:30:33PM -0700, Sean Christopherson wrote:
>> > >  	for (c = 0 ; c < addp.length; c += PAGE_SIZE) {
>> > > -		if (signal_pending(current)) {
>> > > -			ret = -EINTR;
>> > > +		if (c == SGX_MAX_ADD_PAGES_LENGTH || signal_pending(current)) {
>> > > +			ret = c;
>> >
>> > I don't have an opinion on returning count vs. EINTR, but I don't see  
>> the
>> > point in arbitrarily capping the number of pages that can be added in  
>> a
>> > single ioctl().  It doesn't provide any real protection, e.g.  
>> userspace
>> > can simply restart the ioctl() with updated offsets and continue  
>> spamming
>> > EADDs.  We are relying on other limits, e.g. memcg, rlimits, etc... to
>> > reign in malicious/broken userspace.
>> >
>> > There is nothing inherently dangerous about spending time in the  
>> kernel so
>> > long as appropriate checks are made, e.g. for a pending signel and  
>> resched.
>> > If we're missing checks, adding an arbitrary limit won't fix the  
>> underlying
>> > problem, at least not in a deterministic way.
>> >
>> > If we really want a limit of some form, adding a knob to control the  
>> max
>> > size of an enclave seems like the way to go.  But even that is of  
>> dubious
>> > value as I'd rather rely on existing limits for virtual and physical  
>> memory,
>> > and add a proper EPC cgroup to account and limit EPC memory.
>>
>> It is better to have a contract in the API that the number of processed
>> pages can be less than given, not unlike in syscalls such as write().
>
> That can be handled by a comment, no?  If we want to "enforce" the  
> behavior,
> I'd rather bail out of the loop after a random number of pages than have  
> a
> completely arbitrary limit.  The arbitrary limit will create a contract  
> of
> its own and may lead to weird guest implementations.


I agree with Sean on potential issues with the arbitrary hard coded limit.  
Also returning -EINTR is better way to express to user space that  
operations are interrupted by signal and can be retried, which is a known  
pattern for this kind of situations.

Re: [PATCH] x86/sgx: Roof the number of pages process in SGX_IOC_ENCLAVE_ADD_PAGES

[Jarkko Sakkinen]

On Fri, Sep 11, 2020 at 12:38:59PM -0500, Haitao Huang wrote:
> nOn Fri, 11 Sep 2020 10:51:27 -0500, Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> 
> > On Fri, Sep 11, 2020 at 02:43:15PM +0300, Jarkko Sakkinen wrote:
> > > On Tue, Sep 08, 2020 at 10:30:33PM -0700, Sean Christopherson wrote:
> > > > >  	for (c = 0 ; c < addp.length; c += PAGE_SIZE) {
> > > > > -		if (signal_pending(current)) {
> > > > > -			ret = -EINTR;
> > > > > +		if (c == SGX_MAX_ADD_PAGES_LENGTH || signal_pending(current)) {
> > > > > +			ret = c;
> > > >
> > > > I don't have an opinion on returning count vs. EINTR, but I don't
> > > see the
> > > > point in arbitrarily capping the number of pages that can be added
> > > in a
> > > > single ioctl().  It doesn't provide any real protection, e.g.
> > > userspace
> > > > can simply restart the ioctl() with updated offsets and continue
> > > spamming
> > > > EADDs.  We are relying on other limits, e.g. memcg, rlimits, etc... to
> > > > reign in malicious/broken userspace.
> > > >
> > > > There is nothing inherently dangerous about spending time in the
> > > kernel so
> > > > long as appropriate checks are made, e.g. for a pending signel and
> > > resched.
> > > > If we're missing checks, adding an arbitrary limit won't fix the
> > > underlying
> > > > problem, at least not in a deterministic way.
> > > >
> > > > If we really want a limit of some form, adding a knob to control
> > > the max
> > > > size of an enclave seems like the way to go.  But even that is of
> > > dubious
> > > > value as I'd rather rely on existing limits for virtual and
> > > physical memory,
> > > > and add a proper EPC cgroup to account and limit EPC memory.
> > > 
> > > It is better to have a contract in the API that the number of processed
> > > pages can be less than given, not unlike in syscalls such as write().
> > 
> > That can be handled by a comment, no?  If we want to "enforce" the
> > behavior,
> > I'd rather bail out of the loop after a random number of pages than have
> > a
> > completely arbitrary limit.  The arbitrary limit will create a contract
> > of
> > its own and may lead to weird guest implementations.
> 
> 
> I agree with Sean on potential issues with the arbitrary hard coded limit.
> Also returning -EINTR is better way to express to user space that operations
> are interrupted by signal and can be retried, which is a known pattern for
> this kind of situations.

In read() -EINTR is returned only when zero amount of data is processed.

Otherwise, it returns just the count.

/Jarkko

Re: [PATCH] x86/sgx: Roof the number of pages process in SGX_IOC_ENCLAVE_ADD_PAGES

[Jarkko Sakkinen]

On Fri, Sep 11, 2020 at 08:51:27AM -0700, Sean Christopherson wrote:
> On Fri, Sep 11, 2020 at 02:43:15PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Sep 08, 2020 at 10:30:33PM -0700, Sean Christopherson wrote:
> > > >  	for (c = 0 ; c < addp.length; c += PAGE_SIZE) {
> > > > -		if (signal_pending(current)) {
> > > > -			ret = -EINTR;
> > > > +		if (c == SGX_MAX_ADD_PAGES_LENGTH || signal_pending(current)) {
> > > > +			ret = c;
> > > 
> > > I don't have an opinion on returning count vs. EINTR, but I don't see the
> > > point in arbitrarily capping the number of pages that can be added in a
> > > single ioctl().  It doesn't provide any real protection, e.g. userspace
> > > can simply restart the ioctl() with updated offsets and continue spamming
> > > EADDs.  We are relying on other limits, e.g. memcg, rlimits, etc... to
> > > reign in malicious/broken userspace.
> > > 
> > > There is nothing inherently dangerous about spending time in the kernel so
> > > long as appropriate checks are made, e.g. for a pending signel and resched.
> > > If we're missing checks, adding an arbitrary limit won't fix the underlying
> > > problem, at least not in a deterministic way.
> > > 
> > > If we really want a limit of some form, adding a knob to control the max
> > > size of an enclave seems like the way to go.  But even that is of dubious
> > > value as I'd rather rely on existing limits for virtual and physical memory,
> > > and add a proper EPC cgroup to account and limit EPC memory.
> > 
> > It is better to have a contract in the API that the number of processed
> > pages can be less than given, not unlike in syscalls such as write().
> 
> That can be handled by a comment, no?  If we want to "enforce" the behavior,
> I'd rather bail out of the loop after a random number of pages than have a
> completely arbitrary limit.  The arbitrary limit will create a contract of
> its own and may lead to weird guest implementations.

I don't understand.

It is already a random number given that also signal can cause this.

/Jarkko