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

Jarkko Sakkinen
2020-09-24T22:07:36Z

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.

I did that already for v39. It unconditionally discards noexec
partitions.

I see EMODPE as the key driver for this patch, not noexec partitions.

I.e. post you've done SGX_IOC_ENCLAVE_INIT you are capped when it
comes to permissions.

/Jarkko