Re: [RFC PATCH v3 01/12] x86/sgx: Add mm to enclave at mmap()

From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Andy Lutomirski <luto@kernel.org>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	linux-sgx@vger.kernel.org, Dave Hansen <dave.hansen@intel.com>,
	Cedric Xing <cedric.xing@intel.com>,
	Jethro Beekman <jethro@fortanix.com>,
	"Dr . Greg Wettstein" <greg@enjellic.com>,
	Stephen Smalley <sds@tycho.nsa.gov>
Subject: Re: [RFC PATCH v3 01/12] x86/sgx: Add mm to enclave at mmap()
Date: Tue, 18 Jun 2019 07:11:47 -0700	[thread overview]
Message-ID: <20190618141147.GB13781@linux.intel.com> (raw)
In-Reply-To: <CALCETrXgzzYXrMzcHEYiYE+tvCzU3vFErdzoDEMhMXN_qkQXQw@mail.gmail.com>

On Mon, Jun 17, 2019 at 04:42:59PM -0700, Andy Lutomirski wrote:
> On Mon, Jun 17, 2019 at 3:24 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > The enclave mm tracking is currently broken:
> >
> >   - Adding current->mm during ECREATE is wrong as there is no guarantee
> >     that the current process has mmap()'d the enclave, i.e. there may
> >     never be an associated sgx_vma_close() to drop the encl_mm.
> >
> >   - Adding mm's at sgx_vma_open() is wrong as vm_ops->open is called
> >     only when splitting or duplicating a vma.  If userspace performs a
> >     single mmap() on the enclave then SGX will fail to track the mm.
> >     This bug is partially hidden by tracking current->mm at ECREATE.
> >
> > Rework the tracking to get/add the mm at mmap().  A side effect of the
> > bug fix is that sgx_vma_{open,close}() should never encounter a vma with
> > an associated enclave and no associated encl_mm, i.e. WARN if an encl_mm
> > cannot be found in either condition.
> >
> 
> It would be nifty if you could also kill .vm_close, since then VMAs
> could be merged properly.  Would this be straightforward?

Hmm, we probably could move the mm tracking to f_op->{open,release}.  The
downside to that approach is that EPC reclaim would unnecessarily walk the
vmas for processes that have opened the enclave but not mapped any EPC
pages.  In the grand scheme, that's a minor issue and probably worth the
tradeoff of vma merging.

On the plus side, in addition to zapping ->close, I think it would allow
for a simpler vma walking scheme.  Maybe.