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

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

On Thu, Sep 24, 2020 at 12:39:24PM -0700, Dave Hansen wrote:
> On 9/24/20 12:28 PM, Sean Christopherson wrote:
> > On Thu, Sep 24, 2020 at 02:11:37PM -0500, Haitao Huang wrote:
> >> On Wed, 23 Sep 2020 08:50:56 -0500, Jarkko Sakkinen
> >> <jarkko.sakkinen@linux.intel.com> wrote:
> >>> I'll categorically deny noexec in the next patch set version.
> >>>
> >>> /Jarkko
> >> There are use cases supported currently in which enclave binary is received
> >> via IPC/RPC and held in buffers before EADD. Denying noexec altogether would
> >> break those, right?
> > No.  noexec only applies to file-backed VMAs, what you're describing is loading
> > an enclave from an anon VMA, which will still have VM_MAYEXEC.
> 
> Maybe I'm just stupid, but I still don't get the scenario that's being
> thwarted or why it is valuable.  The SDM is worthless on what EMODPE
> does or what its restrictions are.
> 
> 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).

The ->mprotect() hook gives us direct line of sight to such LSM upcalls,
e.g. I have fully working code that implements the LSM integration.  LSM
support isn't in this series because the only thing everyone could agree on
in terms of an LSM implementation is that it wasn't needed for initial
upstreaming :-)