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

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

On Wed, May 22, 2019 at 07:35:17PM -0700, Sean Christopherson wrote:
> 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.

I'm lost with the constraints we want to set.

We can still support fork() if we take a step back from v20 and require
the mmap(). Given the recent comments, I'd guess that is the best
compromise i.e. multiple processes can still share an enclave within
the limitations of ancestor hierarchy. Is this the constraint we agree
now upon? Some emails are a bit contradicting in this sense.

> 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.

Dropped off here completely. What if the mmap() is done before any of
the EADD operations?

> 
> 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;
> }

This looks flakky and error prone. You'd better have some "shadow VMAs"
and check that you have such matching size of the VMA you try to mmap()
and check flags from that.

Who would call this function anyhow and when?

Would be better to first agree on constraints. I have zero idea within
which kind of enviroment this snippet would live e.g.

- mmap() (before, after?)
- multi process constraint (only fork or full on versatility)

/Jarkko