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

[Sean Christopherson <sean.j.christopherson@intel.com>]

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
>