Re: [PATCH v38 10/24] mm: Add vm_ops->mprotect()

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Thu, Sep 24, 2020 at 01:10:31PM -0700, Dave Hansen wrote:
> On 9/24/20 1:01 PM, Sean Christopherson wrote:
> >> In pseudo-C, it's something logically like this for the "nice" case:
> >>
> >> 	ptr = mmap("/some/executable", PROT_EXEC);
> >> 	ioctl(sgx_fd, ADD_ENCLAVE_PAGE, SGX_PROT_EXEC, ptr, size);
> >> 	mmap(sgx_fd);
> >> 	EENTER;
> >>
> >> And we're trying to thwart:
> >>
> >> 	ptr = mmap("/mnt/noexec/file", PROT_READ);
> >> 	ioctl(sgx_fd, ADD_ENCLAVE_PAGE, SGX_PROT_EXEC, ptr, size);
> >> 	mmap(sgx_fd);
> >> 	EENTER;
> >>
> >> because that loads data into the enclave which is executable but which
> >> was not executable normally.  But, we're allowing this from anonymous
> >> memory, so this would seem to work:
> >>
> >> 	ptr = mmap("/mnt/noexec/file", PROT_READ);
> >> 	buffer = malloc(PAGE_SIZE);
> >> 	memcpy(buffer, ptr, PAGE_SIZE);
> >> 	// need mprotect(buf, PROT_EXEC)???
> >> 	ioctl(sgx_fd, ADD_ENCLAVE_PAGE, SGX_PROT_EXEC, buffer, size);
> >> 	mmap(sgx_fd);
> >> 	EENTER;
> >>
> >> and give the same result.  What am I missing?
> > The last example, where the enclave is copied to a buffer, is out of scope
> > for noexec.  But, it is in scope for LSMs, e.g. for this last example, we
> > could add an LSM upcall so that SELinux could require PROCESS_EXECMEM (or an
> > SGX specific equivalent).
> 
> Why don't we just declare enclave memory as "out of scope for noexec" in
> the same way that anonymous memory is, and just discard this patch?
> That doesn't seem too much of a stretch.

Because we lose line of sight to LSM support.  Without enforcing "declare perms
at load time" in the initial series, we would create an ABI where userspace
could load an enclave page with only READ permissions and then map the enclave
with whatever permissions it wants, without any convenient way for SGX to call
into the LSM.

Retroactively enforcing permissions at load time would break the ABI, or at
least yield different behavior based on the mere existence of LSMs, e.g. if
LSMs are supported, suddenly the ADD_PAGES w/ READ -> mmap(RWX) flow breaks,
even if there is no LSM policy denying that behavior.

Enforcing LSM policies using the existing mmap()/mprotect() hooks doesn't work
well because the only information available is a fd pointing at
/dev/sgx/enclave, which is largely useless because /dev/sgx/enclave must be
map SHARED w/ RWX to run an enclave.  We explored things like grabbing a
reference to the source file for later verification, but that means pinning
files for the entire lifetime of an enclave.

Enforcing noexec was an easy/obvious addition since we need 99% of the code for
potential LSM support anyways.