Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)

[Sean Christopherson]

On Wed, May 29, 2019 at 10:38:06PM -0700, Xing, Cedric wrote:
> > From: Christopherson, Sean J
> > Sent: Tuesday, May 28, 2019 2:41 PM
> > 
> > On Tue, May 28, 2019 at 01:48:02PM -0700, Andy Lutomirski wrote:
> > > On Tue, May 28, 2019 at 1:24 PM Sean Christopherson
> > > <sean.j.christopherson@xxxxxxxxx> wrote:
> > > >
> > > > Actually, I think we do have everything we need from an LSM perspective.
> > > > LSMs just need to understand that sgx_enclave_load() with a NULL vma
> > > > implies a transition from RW.  For example, SELinux would interpret
> > > > sgx_enclave_load(NULL, RX) as requiring FILE__EXECMOD.
> > >
> > > You lost me here.  What operation triggers this callback?  And
> > > wouldn't sgx_enclave_load(NULL, RX) sometimes be a transition from RO
> > > or just some fresh executable zero bytes?
> > 
> > An explicit ioctl() after EACCEPTCOPY to update the allowed permissions.
> > For all intents and purposes, the EAUG'd page must start RW.  Maybe a better way to phrase
> > it is that at some point the page must be writable to have any value whatsover.
> > EACCEPTCOPY explicitly requires the page to be at least RW.  EACCEPT technically doesn't
> > require RW, but a RO or RX zero page is useless.  Userspace could still EACCEPT with RO or
> > RX, but SGX would assume a minimum of RW for the purposes of the LSM check.
> 
> Why is an explicit ioctl() necessary after EACCEPTCOPY? Or why is mprotect() not sufficient?

Ignore this, I was trying to avoid having to add a vm_ops mprotect(),
which Andy pointed out was silly.

> > In theory, it's still your MAXPERM model, but with the unnecessary states removed and the
> > others enforced/handled by the natural SGX transitions instead of explictly in ioctls.
> > Underneath the hood the SGX driver would still need to track the MAXPERM.
> 
> What are the "unnecessary states" removed? 

Andy proposed taking full RWX in MAXPERMs, but really we only need "can
writes ever happen to this page", as that allows the SGX driver to avoid
having to track if a page has been mapped PROT_WRITE by any VMA in any
process.

> I'm not sure understand the proposal fully. The whole thing looks to me like
> the driver is undertaking things that should/would otherwise be done by
> mmap()/mprotect() syscalls. It also imposes unnecessary restrictions on user
> mode code, such as mmap(PROT_NONE), ACTIVATE_REGION can be called only once,
> etc. What'd happen if ACTIVATE_REGION is called with a range spanning
> multiple/partial VMAs? What'd happen if an enclave was unmapped than mapped
> again? I'd say the proposal is unintuitive at least.
> 
> In theory, if the driver can keep track of MAXPERM for all pages within an
> enclave, then it could fail mmap() if the requested prot conflicts with any
> page's MAXPERM within that range. Otherwise, MAXPERM could be copied into
> VM_MAY* flags then mprotect() will just follow through. Wouldn't that be a
> much simpler and more intuitive approach?

Ignore all this, again I was trying to avoid hooking mprotect().