linux-kernel - Re: [PATCH v38 10/24] mm: Add vm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1b1bd1f8-210d-486a-b038-2d6eea9d35cd@intel.com>
Date:   Tue, 29 Sep 2020 07:24:24 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        Andy Lutomirski <luto@...capital.net>
Cc:     Sean Christopherson <sean.j.christopherson@...el.com>,
        Haitao Huang <haitao.huang@...ux.intel.com>,
        Andy Lutomirski <luto@...nel.org>, X86 ML <x86@...nel.org>,
        linux-sgx@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Matthew Wilcox <willy@...radead.org>,
        Jethro Beekman <jethro@...tanix.com>,
        Darren Kenny <darren.kenny@...cle.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        asapek@...gle.com, Borislav Petkov <bp@...en8.de>,
        "Xing, Cedric" <cedric.xing@...el.com>, chenalexchen@...gle.com,
        Conrad Parker <conradparker@...gle.com>, cyhanish@...gle.com,
        "Huang, Haitao" <haitao.huang@...el.com>,
        Josh Triplett <josh@...htriplett.org>,
        "Huang, Kai" <kai.huang@...el.com>,
        "Svahn, Kai" <kai.svahn@...el.com>, Keith Moyer <kmoy@...gle.com>,
        Christian Ludloff <ludloff@...gle.com>,
        Neil Horman <nhorman@...hat.com>,
        Nathaniel McCallum <npmccallum@...hat.com>,
        Patrick Uiterwijk <puiterwijk@...hat.com>,
        David Rientjes <rientjes@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>, yaozhangx@...gle.com
Subject: Re: [PATCH v38 10/24] mm: Add vm_ops->mprotect()

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?