lkml.org 
[lkml]   [2020]   [Sep]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v38 10/24] mm: Add vm_ops->mprotect()
    From
    Date
    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?

    \
     
     \ /
      Last update: 2020-09-29 16:25    [W:4.409 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site