Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From:   Andy Lutomirski <luto@amacapital.net>
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH v38 10/24] mm: Add vm_ops->mprotect()
Date:   Fri, 18 Sep 2020 17:15:32 -0700
Message-Id: <1B23E216-0229-4BDD-8B09-807256A54AF5@amacapital.net>
References: <20200918235337.GA21189@sjchrist-ice>
Cc:     Andy Lutomirski <luto@kernel.org>,
        Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
        X86 ML <x86@kernel.org>, linux-sgx@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>,
        Linux-MM <linux-mm@kvack.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Matthew Wilcox <willy@infradead.org>,
        Jethro Beekman <jethro@fortanix.com>,
        Darren Kenny <darren.kenny@oracle.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        asapek@google.com, Borislav Petkov <bp@alien8.de>,
        "Xing, Cedric" <cedric.xing@intel.com>, chenalexchen@google.com,
        Conrad Parker <conradparker@google.com>, cyhanish@google.com,
        Dave Hansen <dave.hansen@intel.com>,
        "Huang, Haitao" <haitao.huang@intel.com>,
        Josh Triplett <josh@joshtriplett.org>,
        "Huang, Kai" <kai.huang@intel.com>,
        "Svahn, Kai" <kai.svahn@intel.com>, Keith Moyer <kmoy@google.com>,
        Christian Ludloff <ludloff@google.com>,
        Neil Horman <nhorman@redhat.com>,
        Nathaniel McCallum <npmccallum@redhat.com>,
        Patrick Uiterwijk <puiterwijk@redhat.com>,
        David Rientjes <rientjes@google.com>,
        Thomas Gleixner <tglx@linutronix.de>, yaozhangx@google.com
In-Reply-To: <20200918235337.GA21189@sjchrist-ice>
To:     Sean Christopherson <sean.j.christopherson@intel.com>
Precedence: bulk


> On Sep 18, 2020, at 4:53 PM, Sean Christopherson <sean.j.christopherson@in=
tel.com> wrote:
>=20
> =EF=BB=BFOn Fri, Sep 18, 2020 at 08:09:04AM -0700, Andy Lutomirski wrote:
>>> On Tue, Sep 15, 2020 at 4:28 AM Jarkko Sakkinen
>>> <jarkko.sakkinen@linux.intel.com> wrote:
>>>=20
>>> From: Sean Christopherson <sean.j.christopherson@intel.com>
>>>=20
>>> Add vm_ops()->mprotect() for additional constraints for a VMA.
>>>=20
>>> Intel Software Guard eXtensions (SGX) will use this callback to add two
>>> constraints:
>>>=20
>>> 1. Verify that the address range does not have holes: each page address
>>>   must be filled with an enclave page.
>>> 2. Verify that VMA permissions won't surpass the permissions of any encl=
ave
>>>   page within the address range. Enclave cryptographically sealed
>>>   permissions for each page address that set the upper limit for possibl=
e
>>>   VMA permissions. Not respecting this can cause #GP's to be emitted.
>=20
> Side note, #GP is wrong.  EPCM violations are #PFs.  Skylake CPUs #GP, but=

> that's technically an errata.  But this isn't the real motivation, e.g.
> userspace can already trigger #GP/#PF by reading/writing a bad address, SG=
X
> simply adds another flavor.
>=20
>> It's been awhile since I looked at this.  Can you remind us: is this
>> just preventing userspace from shooting itself in the foot or is this
>> something more important?
>=20
> Something more important, it's used to prevent userspace from circumventin=
g
> a noexec filesystem by loading code into an enclave, and to give the kerne=
l the
> option of adding enclave specific LSM policies in the future.
>=20
> The source file (if one exists) for the enclave is long gone when the encl=
ave
> is actually mmap()'d and mprotect()'d.  To enforce noexec, the requested
> permissions for a given page are snapshotted when the page is added to the=

> enclave, i.e. when the enclave is built.  Enclave pages that will be execu=
table
> must originate from an a MAYEXEC VMA, e.g. the source page can't come from=
 a
> noexec file system.
>=20
> The ->mprotect() hook allows SGX to reject mprotect() if userspace is decl=
aring
> permissions beyond what are allowed, e.g. trying to map an enclave page wi=
th
> EXEC permissions when the page was added to the enclave without EXEC.
>=20
> Future LSM policies have a similar need due to vm_file always pointing at
> /dev/sgx/enclave, e.g. policies couldn't be attached to a specific enclave=
.
> ->mprotect() again allows enforcing permissions at map time that were chec=
ked
> at enclave build time, e.g. via an LSM hook.
>=20
> Deferring ->mprotect() until LSM support is added (if it ever is) would be=

> problematic due to SGX2.  With SGX2, userspace can extend permissions of a=
n
> enclave page (for the CPU's EPC Map entry, not the kernel's page tables)
> without bouncing through the kernel.  Without ->mprotect () enforcement.
> userspace could do EADD(RW) -> mprotect(RWX) -> EMODPE(X) to gain W+X.  We=

> want to disallow such a flow now, i.e. force userspace to do EADD(RW,X), s=
o
> that the hypothetical LSM hook would have all information at EADD(), i.e.
> would be aware of the EXEC permission, without creating divergent behavior=

> based on whether or not an LSM is active.

That=E2=80=99s what I thought. Can we get this in the changelog?=