Re: [PATCH v2 2/5] x86/sgx: Reject concurrent ioctls on single enclave

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

On Thu, Aug 29, 2019 at 09:14:38PM +0300, Jarkko Sakkinen wrote:
> On Thu, Aug 29, 2019 at 09:00:01AM -0700, Sean Christopherson wrote:
> > > #PF handler should be good as it has this conditional:
> > > 
> > > flags = atomic_read(&encl->flags);
> > > 
> > > if ((flags & SGX_ENCL_DEAD) || !(flags & SGX_ENCL_INITIALIZED))
> > > 	return ERR_PTR(-EFAULT);
> > > 
> > > What about the reclaimer?
> > 
> > Can you elaborate?  I'm not sure what you're asking.
> 
> I'm thinking of a race between list_add() in the ioctl and
> list_move_tail() in the reclaimer.

Ah crud, I forgot that the reclaimer can manipulate the list of VA pages,
I was thinking they were invisible to the reclaimer.

> A quick way to fix this would be move sgx_alloc_va_page() from
> sgx_encl_grow() and return NULL if a new allocation is required.

We don't even need to do that, moving the list_add() from sgx_encl_grow()
to its caller would be sufficient.  Same concept, but the allocation would
be handled by sgx_encl_grow() instead of having to duplicate that code in
sgx_encl_add_page() and sgx_encl_create().

> In the ioctl you can then allocate the page before taking locks and
> do "list_add(&va_page->list, &encl->va_pages);" behind the locks.