On 9/28/20 9:05 PM, Jarkko Sakkinen wrote:
> On Mon, Sep 28, 2020 at 06:37:54PM -0700, Andy Lutomirski wrote:
>> I don’t personally care that much about EMODPE but, you could probably
>> get the point across with something like:
>>
>> SGX’s EPCM permission bits do not obviate the need to enforce these
>> rules in the PTEs because enclaves can freely modify the EPCM
>> permissions using EMODPE.
>>
>> IOW, EMODPE is not really special here; rather, EMODPE’s existence
>> demonstrates that EADD / EEXTEND are not special.
>
> So I did "disagree and commit" with this one. I'm not actually
> diagreeing on anything what Dave wrote, on the contrary it is an
> understandable high level description. I just thought that it would not
> hurt to remark that the ISA contains such peculiarities as EMODPE.
>
> I did only very rudimentary clean up for the text (e.g. fix the ioctl
> name, add shortt summary and not much else).
>
> Does not make sense to waste more time to this. I'll move on to
> implement the missing boot time patching for the vDSO so that we
> get the next version out.
>
> "
> mm: Add 'mprotect' hook to struct vm_operations_struct
>
> Background
> ==========
>
> 1. SGX enclave pages are populated with data by copying data to them
> from normal memory via ioctl(fd, SGX_IOC_ENCLAVE_ADD_PAGES).
> 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.
I know I wrote that. I suck, and I wrote it in a changelog-unacceptable
way. Folks dislike the use of "we" in these things. Here's a better
version:
2. It is desirable to be able to restrict those normal memory data
sources. For instance, the kernel can ensure that the source is
executable, before copying data to an executable enclave page.
> 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(fd, SGX_IOC_ENCLAVE_ADD_PAGES). 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.
Let's also say "... or *requested* by the enclave itself ...", since the
enclave itself can't directly make syscalls.
> 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(fd, SGX_IOC_ENCLAVE_ADD_PAGES). That means that the intent
> argument (SGX_PROT_*) is currently unused.
This was incorrect to say. Sean corrected me on this point. Could you
look through the thread and incorporate that?
