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?
