Re: [PATCH] x86/sgx: rename and document SGX bit lock

[Jarkko Sakkinen <jarkko@kernel.org>]

On Tue, Jan 12, 2021 at 02:19:01PM -0800, Dave Hansen wrote:
> 
> SGX ioctl() calls are serialized with a lock.  It's a weird open-coded
> lock that is not even called a "lock".  That makes it a weird beast,
> but Sean has convinced me it's a good idea without better alternatives.
> 
> Give the lock bit a better name, and document what it actually trying
> to do.
> 
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: x86@kernel.org
> 
> ---
> 
>  b/arch/x86/kernel/cpu/sgx/encl.h  |    2 +-
>  b/arch/x86/kernel/cpu/sgx/ioctl.c |   19 ++++++++++++++++---
>  2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff -puN arch/x86/kernel/cpu/sgx/ioctl.c~sgx-encl-flags arch/x86/kernel/cpu/sgx/ioctl.c
> --- a/arch/x86/kernel/cpu/sgx/ioctl.c~sgx-encl-flags	2021-01-12 14:02:24.480689006 -0800
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c	2021-01-12 14:02:24.486689006 -0800
> @@ -690,8 +690,21 @@ long sgx_ioctl(struct file *filep, unsig
>  	struct sgx_encl *encl = filep->private_data;
>  	int ret;
>  
> -	if (test_and_set_bit(SGX_ENCL_IOCTL, &encl->flags))
> -		return -EBUSY;
> +	/*
> +	 * Behold, the Big SGX Lock
> +	 *
> +	 * The primary function of this "lock" is to actively discourage
> +	 * attempts at multi-threaded enclave management.  Enclave management
> +	 * is fundamentally a single-threaded affair.  Enclave measurement,
> +	 * for instance would be worthless if two ADD_PAGES instances raced
> +	 * and occurred in different orders.
> +	 *
> +	 * encl->lock is ill suited for this because it would need to be
> +	 * conditionally dropped and reqacuired for operations like enclave
> +	 * page allocation and reclaim.
> +	 */
> +	if (test_and_set_bit(SGX_ENCL_IOCTL_LOCK, &encl->flags))
> +		return -EINVAL;

Precisely this come down to SGX_IOC_ENCLAVE_ADD_PAGES ioctl where
you need to do multiple sgx_alloc_epc_pages() calls. Other ioctl's
are not bound to this.

In other words, two threads could have ABBA race if they wait for each
other locks while swapping. Since cryptographic measurements require
strict order anyway, this is a simple way to sort out the issue.

Other way to sort this out would be to pre-allocate the amount of
pages and VA pages required to add new pages before taking the
lock but that its own set problems (like the being non-swappable
for a while).

Maybe these details could be sharpened in the comment? I agree with
the name change. I.e.

1. We do this because the add page flow requires this.
2. The order must be sequential anyway so no harm done.
3. Pre-allocating variable number of pages is not an alternative.

/Jarkko

>  
>  	switch (cmd) {
>  	case SGX_IOC_ENCLAVE_CREATE:
> @@ -711,6 +724,6 @@ long sgx_ioctl(struct file *filep, unsig
>  		break;
>  	}
>  
> -	clear_bit(SGX_ENCL_IOCTL, &encl->flags);
> +	clear_bit(SGX_ENCL_IOCTL_LOCK, &encl->flags);
>  	return ret;
>  }
> diff -puN arch/x86/kernel/cpu/sgx/encl.h~sgx-encl-flags arch/x86/kernel/cpu/sgx/encl.h
> --- a/arch/x86/kernel/cpu/sgx/encl.h~sgx-encl-flags	2021-01-12 14:02:24.482689006 -0800
> +++ b/arch/x86/kernel/cpu/sgx/encl.h	2021-01-12 14:16:37.511686879 -0800
> @@ -34,7 +34,7 @@ struct sgx_encl_page {
>  };
>  
>  enum sgx_encl_flags {
> -	SGX_ENCL_IOCTL		= BIT(0),
> +	SGX_ENCL_IOCTL_LOCK	= BIT(0), /* See sgx_ioctl() */
>  	SGX_ENCL_DEBUG		= BIT(1),
>  	SGX_ENCL_CREATED	= BIT(2),
>  	SGX_ENCL_INITIALIZED	= BIT(3),
> _
>