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

["Xing, Cedric" <cedric.xing@intel.com>]

> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@vger.kernel.org] On Behalf Of Sean Christopherson
> Sent: Friday, May 24, 2019 12:14 PM
> 
> My point is that enclaves have different properties than shared objects.
> 
> Normal LSM behavior with regard to executing files is to label files
> with e.g. FILE__EXECUTE.  Because an enclave must be built to the exact
> specifications of .sigstruct, requring FILE__EXECUTE on the .sigstruct
> is effectively the same as requiring FILE__EXECUTE on the enclave itself.
> 
> Addressing your scenario of loading an executable page in EPC, doing so
> would require one of the following:
> 
>   - Ability to install a .sigstruct with FILE__EXECUTE
> 
>   - PROCESS__EXECMEM
> 
>   - FILE__EXECMOD and SGX2 support

Now I got your point. It sounds a great idea to me!

But instead of using .sigstruct file, I'd still recommend using file mapping (i.e. SIGSTRUCT needs to reside in executable memory). But then there'll be a hole - a process having FILE__EXECMOD on any file could use that file as a SIGSTRUCT. Probably we'll need a new type in SELinux to label enclave/sigstruct files.

> 
> > In the case a RWX page is needed, the calling process has to have a
> > RWX page serving as the source for EADD so PROCESS__EXECMEM will have
> been checked.
> > For SGX2, changing an EPC page to RWX is subject to FILE__EXECMEM on
> > /dev/sgx/enclave, which I see as a security benefit because it only
> > affects the enclave but not the whole process hosting it.
> 
> There is no FILE__EXECMEM check, only PROCESS__EXECMEM and FILE__EXECMOD.
> I assume you're referring to the latter?

Yes.

> 
> I don't see a fundamental difference between having RWX in an enclave
> and RWX in normal memory, either way the process can execute arbitrary
> code, i.e. PROCESS__EXECMEM is appropriate.  Yes, an enclave will #UD on
> certain instructions, but that's easily sidestepped by having a
> trampoline in the host (marked RX) and piping arbitrary code into the
> enclave.  Or using EEXIT to do a bit of ROP.

I'm with you.

With your proposal only FILE__EXECMOD is needed on /dev/sgx/enclave to launch Graphene enclaves or the like.

> 
> > > > No changes are required to LSMs, SGX1 has a single LSM touchpoint
> > > > in
> > > its
> > > > mmap(), and I *think* the only required userspace change is to
> > > > mmap() PROT_NONE when allocating the enclave's virtual address
> range.
> >
> > I'm not sure I understand the motivation behind this proposal to
> > decouple initial EPC permissions from source pages.
> 
> Pulling permissions from source pages means userspace needs to fully map
> the in normal memory, including marking pages executable.  That exposes
> the loader to having executable pages in its address space that it has
> no intention of executing (outside of the enclave).  And for Graphene,
> it means having to actively avoid PROCESS__EXECMEM, e.g. by using a
> dummy backing file to build the enclave instead of anon memory.

Agreed.

> 
> > I don't think it a big deal to fully mmap() enclave files, which have
> > to be parsed by user mode anyway to determine various things including
> > but not limited to the size of heap(s), size and number of
> > TCSs/stacks/TLS areas, and the overall enclave size. So with PHDRs
> > parsed, it's trivial to mmap() each segment with permissions from its
> PHDR.
> >
> > > > As for Graphene, it doesn't need extra permissions to run its
> > > > enclaves, it just needs a way to install .sigstruct, which is a
> > > > generic permissions problem and not SGX specific.
> > > >
> > > >
> > > > For SGX2 maybe:
> > > >
> > > >   - No additional requirements to map an EAUG'd page as RW page.
> Not
> > > >     aligned with standard MAP_SHARED behavior, but we really don't
> want
> > > >     to require FILE__WRITE, and thus allow writes to .sigstruct.
> > > >
> > > >   - Require FILE__EXECMOD on the .sigstruct to map previously
> writable
> > > >     page as executable (which indirectly includes all EAUG'd
> pages).
> > > >     Wiring this up will be a little funky, but we again we don't
> want
> > > >     to require FILE__WRITE on .sigstruct.
> > > >
> >
> > I'm lost. Why is EAUG tied to permissions on .sigstruct?
> 
> Because for the purposes of LSM checks, .sigstruct is the enclave's
> backing file, and mapping a previously writable enclave page as
> exectuable is roughly equivalent to mapping a CoW'd page as exectuable.

I think I've got your idea. You are trying to use permissions on .sigstruct to determine whether EAUG will be available to that specific enclave. Am I right?

I'd tie EAUG to the permissions of /dev/sgx/enclave instead. But why? There are couple of reasons. For one, a SIGSTRUCT identifies the behavior of the enclave, hence the SGX features needed by that enclave. So if an enclave requires EAUG, the .sigstruct has to allow EAUG or the enclave wouldn't work. That means the system admin wouldn't have a choice but to match up what's needed by the enclave. For two, whether to allow, say loading code dynamically into an enclave, depends on whether the host process can tolerate the inherent risk. And that decision is seldom made on individual enclaves but to the host process as a whole. And /dev/sgx/enclave serves that purpose.

-Cedric