Re: x86/sgx: v23-rc2

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

On Thu, Oct 10, 2019 at 10:09:21AM -0700, Sean Christopherson wrote:
> On Thu, Oct 10, 2019 at 04:37:53PM +0300, Jarkko Sakkinen wrote:
> > On Thu, Oct 10, 2019 at 02:37:45PM +0300, Jarkko Sakkinen wrote:
> > > tag v23-rc2
> > > Tagger: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > Date:   Thu Oct 10 14:33:07 2019 +0300
> > > 
> > > x86/sgx: v23-rc1 patch set
> > > 
> > > * Return -EIO instead of -ECANCELED when ptrace() fails to read a TCS page.
> > > * In the reclaimer, pin page before ENCLS[EBLOCK] because pinning can fail
> > >   (because of OOM) even in legit behaviour and after EBLOCK the reclaiming
> > >   flow can be only reverted by killing the whole enclave.
> > > * Fixed SGX_ATTR_RESERVED_MASK. Bit 7 was marked as reserved while in fact
> > >   it should have been bit 6 (Table 37-3 in the SDM).
> > > * Return -EPERM from SGX_IOC_ENCLAVE_INIT when ENCLS[EINIT] returns an SGX
> > >   error code.
> > > * In v22 __uaccess_begin() was used to pin the source page in
> > >   __sgx_encl_add_page(). Switch to get_user_pages() in order to avoid
> > >   deadlock (mmap_sem might get locked twice in the same thread).
> > 
> > __uaccess_begin() is also needed to performan access checks the legit
> 
> __uaccess_begin() doesn't check the address space, it temporarily disables
> SMAP/SMEP so that the kernel can access a user mapping.  An explicit
> access_ok() call should be added as well.
> 
> > user space address. What we can do is to use get_user_pages() just to
> > make sure that the page is faulted while we perform ENCLS[EADD].
> 
> 
> > I updated the master branch with the fix for this. Now the access
> > pattern is:
> > 
> > 	ret = get_user_pages(src, 1, 0, &src_page, NULL);
> > 	if (ret < 1)
> > 		return ret;
> > 
> > 	__uaccess_begin();
> 
> This should be immediately before __eadd().  I also think it'd be a good
> idea to disable page faults around __eadd() so that an unexpected #PF
> manifests as an __eadd() failure and not a kernel hang.

Scratch that, using the userspace is address is flat out wrong.  gup()
doesn't guarantee the userspace mapping is valid, i.e. EADD can still get
a #PF and deadlock.  AIUI, the correct/bulletproof approach is to kmap()
after gup() and use the kernel translation.