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

[Dave Hansen]

Thanks for the walkthrough.  The thing that clicked for me seeing those
examples was how the earlier ioctl(ADD_PAGE) is "bound" to later
enforcement actions at enclave PTE creation time.

On 9/24/20 5:00 PM, Sean Christopherson wrote:
> My concern is that if we merge this
> 
>         ioctl(sgx_fd, ENCLAVE_ADD_PAGE, SGX_PROT_READ | SGX_PROT_EXEC, ptr, size);
> 
> without ->mprotect(), we can't actually enforce the declared protections.  And
> if we drop the field altogether:
> 
>         ioctl(sgx_fd, ENCLAVE_ADD_PAGE, ptr, size);
> 
> then we can't implement security_enclave_load().

To me, it's perfectly OK to have parts of the ABI which are unused.  It
sure makes them harder to test if there are no actual users in the code,
but if it solves a real problem with the ABI, I'm fine with it.

Let's see if I can put all the pieces together.

Background:

1. SGX enclave pages are populated with data by copying data to them
   from normal memory via: ioctl(sgx_fd, ENCLAVE_ADD_PAGE, src_ptr...);
2. We want to be able to restrict those normal memory data sources.  For
   instance, before copying data to an executable enclave page, we might
   ensure that the source is executable.
3. Enclave page permissions are dynamic just like normal permissions and
   can be adjusted at runtime with mprotect() (along with a
   corresponding special instruction inside the enclave)
4. The original data source may have have long since vanished at the
   time when enclave page permission are established (mmap() or
   mprotect())

Solution:

The solution is to force enclaves creators to declare their intent up
front to ioctl(ENCLAVE_ADD_PAGE).  This intent can me immediately
compared to the source data mapping (and rejected if necessary).  It is
also stashed off and then later compared with enclave PTEs to ensure
that any future mmap()/mprotect() operations performed by the enclave
creator or the enclave itself are consistent with the earlier declared
permissions.

Essentially, this means that whenever the kernel is asked to change an
enclave PTE, it needs to ensure the change is consistent with that
stashed intent.  There is an existing vm_ops->mmap() hook which allows
SGX to do that for mmap().  However, there is no ->mprotect() hook.  Add
a vm_ops->mprotect() hook so that mprotect() operations which are
inconsistent with any page's stashed intent can be rejected by the driver.

Implications:

However, there is currently no implementation of the intent checks at
the time of ioctl(ENCLAVE_ADD_PAGE).  That means that the intent
argument (SGX_PROT_*) is currently unused.

--

Is that all correct?  Did I miss anything?