Re: [PATCH 3/4] x86/sgx: Reject concurrent ioctls on single enclave

From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: linux-sgx@vger.kernel.org
Subject: Re: [PATCH 3/4] x86/sgx: Reject concurrent ioctls on single enclave
Date: Tue, 27 Aug 2019 15:11:25 +0300	[thread overview]
Message-ID: <20190827121125.rmcsf2qlicb4xduf@linux.intel.com> (raw)
In-Reply-To: <20190827001128.25066-4-sean.j.christopherson@intel.com>

On Mon, Aug 26, 2019 at 05:11:27PM -0700, Sean Christopherson wrote:
> Except for ENCLAVE_SET_ATTRIBUTE, all SGX ioctls() must be executed
> serially to successfully initialize an enclave, e.g. the kernel already
> strictly requires ECREATE->EADD->EINIT, and concurrent EADDs will result
> in an unstable MRENCLAVE.  Explicitly enforce serialization by returning
> EINVAL if userspace attempts an ioctl while a different ioctl for the
> same enclave is in progress.
> 
> The primary beneficiary of explicit serialization is sgx_encl_grow() as
> it no longer has to deal with dropping and reacquiring encl->lock when
> a new VA page needs to be allocated.  Eliminating the lock juggling in
> sgx_encl_grow() paves the way for fixing a lock ordering bug in
> ENCLAVE_ADD_PAGE without having to also juggle mm->mmap_sem.
> 
> Serializing ioctls also fixes a race between ENCLAVE_CREATE and
> ENCLAVE_SET_ATTRIBUTE, as the latter does not take encl->lock, e.g.
> concurrent updates to allowed_attributes could result in a stale
> value.  The race could also be fixed by taking encl->lock, but that
> is less desirable as doing so would unnecessarily interfere with EPC
>b page reclaim.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

I think the error code should be -EBUSY. Our implementation is fairly
coherent at the moment that -EINVAL follows from bad input data. Getting
-EINVAL from this would be a bit confusing.

If atomic_t was used for the enclave flags (see my comment to 1/4), then
I think we could implement this like:

if (atomic_fetch_or(SGX_ENCL_IOCTL, &encl->flags) & SGX_ENCL_IOCTL)
	return -EIOCTL;

/Jarkko