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

[Sean Christopherson]

On Wed, May 22, 2019 at 03:42:45PM -0700, Andy Lutomirski wrote:
> On Wed, May 22, 2019 at 8:38 AM Sean Christopherson
> <sean.j.christopherson@xxxxxxxxx> wrote:
> >
> > And that straight up doesn't work with the v20 driver because mmap() with
> > the enclave_fd will run through sgx_get_unmapped_area(), which also does
> > the natural alignment adjustments (the idea being that mmap() is mapping
> > the entire enclave).  E.g. mmap() will map the wrong address if the offset
> > of a chunk is less than its size due to the driver adjusting the address.
> 
> That presumably needs to change.

If we want to allow mmap() on a subset of the enclave, yes.  I assume it's
a simple matter of respecting MAP_FIXED.

> Are we entirely missing an API to allocate a naturally aligned VA
> range?  That's kind of annoying.

Yes?

> > Eliminating sgx_get_unmapped_area() means userspace is once again on the
> > hook for naturally aligning the enclave, which is less than desirable.
> >
> > Looking back at the original API discussions around a builder process[1],
> > we never fleshed out the end-to-end flow.  While having a builder process
> > *sounds* reasonable, in practice it adds a lot of complexity without
> > providing much in the way of added security.  E.g. in addition to the
> > above mmap() issues, since the order of EADDs affects the enclave
> > measurement, the enclave owner would need to communicate the exact steps
> > to build the enclave, or the builder would need a priori knowledge of the
> > enclave format.
> >
> > Userspace can still restrict access to /dev/sgx/enclave, e.g. by having a
> > daemon that requires additional credentials to obtain a new enclave_fd.
> > So AFAICT, the only benefit to having a dedicated builder is that it can
> > do its own whitelisting of enclaves, but since we're trending towards
> > supporting whitelisting enclaves in the kernel, e.g. via sigstruct,
> > whitelisting in userspace purely in userspace also provides marginal value.
> >
> > TL;DR: Requiring VMA backing to build an enclave seems reasonable and sane.
> 
> This isn't necessarily a problem, but we pretty much have to use
> mprotect() then.

You lost me there.  Who needs to mprotect() what?

> Maybe the semantics could just be that mmap() on the SGX device gives
> natural alignment, but that there is no actual constraint enforced by
> the driver as to whether mmap() happens before or after ECREATE.
> After all, it's *ugly* for user code to reserve its address range with
> an awkward giant mmap(), there's nothing fundamentally wrong with it.
> 
> As far as I know from this whole discussion, we still haven't come up
> with any credible way to avoid tracking, per enclave page, whether
> that page came from unmodified PROT_EXEC memory.

Disallowing mmap() after ECREATE is credible, but apparently not
palatable. :-)

But actually, there's no need to disallow mmap() after ECREATE since the
LSM checks also apply to mmap(), e.g. FILE__EXECUTE would be needed to
mmap() any enclave pages PROT_EXEC.  I guess my past self thought mmap()
bypassed LSM checks?  The real problem is that mmap()'ng an existing
enclave would require FILE__WRITE and FILE__EXECUTE, which puts us back
at square one.

Tracking permissions per enclave page isn't difficult, it's the new SGX
specific LSM hooks and mprotect() interactions that I want to avoid.

Jumping back to mmap(), AIUI the fundamental issue is that we want to
allow building/running an enclave without FILE__WRITE and FILE__EXECUTE,
otherwise FILE__WRITE and FILE__EXECUTE become meaningless.  Assuming I'm
not off in the weeds, that means we really just need to special case
mmap() on enclaves so it can map enclave memory using the verified page
permissions so as not to run afoul of LSM checks.  All other behaviors,
e.g. mprotect(), can reuse the existing LSM checks for shared mappings.

So, what if we snapshot the permissions for each enclave page at EADD,
and then special case mmap() to propagate flags from the snapshot to the
VMA?  More or less the same idea as doing mprotect_fixup() using the
source VMA during EADD.  We could define the EADD semantics to match
this as well, e.g. only propagate the flags from the source VMA to the
enclave VMA if the EADD range is fully mapped with PROT_NONE.  This would
allow the enclave builder concept, albeit with funky semantics, and
wouldn't require new LSM hooks.

E.g. something like this:

static inline void sgx_mmap_update_prot_flags(struct vm_area_struct *vma,
					      struct sgx_encl *encl)
{
	struct radix_tree_iter iter;
	struct sgx_encl_page *entry;
	unsigned long addr;
	vm_flags_t flags;
	void **slot;

	/*
	 * SGX special: if userspace is requesting PROT_NONE and pages have
	 * been added to the enclave, then propagate the flags snapshot from
	 * the enclave to the VMA.  Do this if and only if all overlapped
	 * pages are defined and have identical permissions.  Stuffing the
	 * VMA on PROT_NONE allows userspace to map EPC pages without being
	 * incorrectly rejected by LSMs due to insufficient permissions (the
	 * snapshottted flags have alaredy been vetted).
	 */
	if (vma->vm_flags & (VM_READ|VM_WRITE|VM_EXEC))
		return;

	flags = 0;

	for (addr = vma->vm_start; addr < vma->vm_end; addr += PAGE_SIZE) {
		entry = radix_tree_lookup(&encl->page_tree, addr >> PAGE_SHIFT);

		if (!entry && flags)
			return;
		if (!flags && entry) {
			if (addr == vma->vm_start) {
				flags = entry->vm_flags;
				continue;
			}
			return;
		}
		if (entry && flags && entry->vm_flags != flags)
			return;

	}
	vma->vm_flags |= flags;
}