All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Dr. Greg" <greg@enjellic.com>
To: Andy Lutomirski <luto@kernel.org>
Cc: Dave Hansen <dave.hansen@intel.com>,
	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	X86 ML <x86@kernel.org>,
	linux-sgx@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
	Sean Christopherson <sean.j.christopherson@intel.com>,
	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, "Huang, Haitao" <haitao.huang@intel.com>,
	"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, Mikko Ylinen <mikko.ylinen@intel.com>
Subject: Re: [PATCH v40 10/24] mm: Add 'mprotect' hook to struct vm_operations_struct
Date: Mon, 16 Nov 2020 12:00:23 -0600	[thread overview]
Message-ID: <20201116180023.GA32481@wind.enjellic.com> (raw)
In-Reply-To: <CALCETrWaUDO1eG7PE_bpA2C_OVeNZ7VbEVaznkg2U7Qx=X=oEw@mail.gmail.com>

On Thu, Nov 12, 2020 at 02:41:00PM -0800, Andy Lutomirski wrote:

Good morning, I hope the week is starting well for everyone.

> On Thu, Nov 12, 2020 at 1:31 PM Dave Hansen <dave.hansen@intel.com> wrote:
> >
> > On 11/12/20 12:58 PM, Dr. Greg wrote:
> > > @@ -270,11 +270,10 @@ static int sgx_vma_mprotect(struct vm_area_struct *vma,
> > >                           struct vm_area_struct **pprev, unsigned long start,
> > >                           unsigned long end, unsigned long newflags)
> > >  {
> > > -     int ret;
> > > +     struct sgx_encl *encl = vma->vm_private_data;
> > >
> > > -     ret = sgx_encl_may_map(vma->vm_private_data, start, end, newflags);
> > > -     if (ret)
> > > -             return ret;
> > > +     if ( test_bit(SGX_ENCL_INITIALIZED, &encl->flags) )
> > > +             return -EACCES;
> > >
> > >       return mprotect_fixup(vma, pprev, start, end, newflags);
> > >  }
> >
> > This rules out mprotect() on running enclaves.  Does that break any
> > expectations from enclave authors, or take away capabilities that folks
> > need?

> It certainly prevents any scheme in which an enclave coordinates
> with the outside world to do W-and-then-X JIT inside.  I'm also not
> convinced it has any real effect at all unless there's some magic I
> missed to prevent someone from using mmap(2) to effectively change
> permissions.

The patch that I posted yesterday addresses the security issue for
both mmap and mprotect by trapping the permission change request at
the level of the sgx_encl_may_map() function.

With respect to the W-and-then-X JIT issue, the stated purpose of the
driver is to implement basic SGX functionality, which is SGX1
semantics, it has been stated formally for a year by the developers
themselves that they are not entertaining a driver that addresses any
of the issues associated with non-static memory permissions.

As I've noted previously, the hardware itself is capable of enforcing
that after initialization, if mmap/mprotect is blocked on hardware
that supports SGX2 instructions.

> Everyone, IMO this SGX1 - vs - SGX2 - vs - EDMM discussion is
> entirely missing the point and is a waste of everyone's time.  Let's
> pretend we're building a system that has nothing to do with SGX and
> requires no special hardware support at all.  It works like this:

I don't doubt there is a potential bigger vision here, quite frankly
it is probably an open question whether or not SGX is going to be a
part of this future, for a variety of reasons.  I also do not doubt
that you have the skills to define that vision.

Right now, however, the issue is not about pretending but rather one
of getting a driver into the kernel that provides a framework for
building whatever future SGX may have.  Given GKH's comments on LWN
last week in response to the RAPL vulnerability, I'm not sure if it is
a politically done deal that the driver will go in.

SGX has specific hardware characteristics that impact the driver, I
don't see how fitting it into a generic trusted execution model
advances the agenda immediately at hand.  Particularly given the fact
that I'm not even sure people understand the questions that need to be
answered about such a generic model.

> A user program opens /dev/xyz and gets back an fd that represents 16
> MB of memory.  The user program copies some data from disk (or
> network or whatever) into fd (using write(2) or ioctl(2) or mmap(2)
> and memcpy) and then mmaps some of the fd as R and some as RW and
> some as RX, and then the user program jumps into the RX mapping.

This is basically the SGX model in the new driver.  The important
defining characteristic of the driver, that we can't wave away, is
that the hardware requires a specific set of initial page permissions
to be implemented in order for initialization of the memory range
(enclave) to succeed.

This is inherent to the way SGX hardware was designed to work.  The
only difference between SGX1 and SGX2 is that the latter offers a
small number of additional instructions that allow the page
permissions to be dynamically manipulated after initialization is
complete.

From a security perspective, the issue at hand is that the executable
material is not going to come in through the fd, it is going to be
loaded by the enclave over the network.  This isn't fear mongering, it
is the stated intent of what people want to do with the technology as
a integral part of confidential computing.

I've had the opportunity to brief DOD and other entities concerned
with intelligence issues, about these type of potential capabilities.
It isn't hard to envision scenarios of where having potentially
sensitive code and data only ever handled and executed by a trusted
entity, in an environment that is inherently ephemeral with respect to
its persistence, is an important design characteristic.  Thermite has
also been known to play a role in some of these designs prior to the
greater elegance of trusted execution environments.

Ultimately, if you believe the Confidential Computing Consortium, it
is also what people want for their sensitive cloud workloads.  Absent
the thermite of course.

> If we replace /dev/xyz with /dev/zero, then this simply does not work
> under a reasonably strict W^X policy -- a lot of people think it's
> quite reasonable for an OS to prevent a user program from obtaining an
> X mapping containing anything other than a mapping from a file on
> disk.  To solve this, we can do one of at least three things:
> 
> a) You can't use /dev/xyz unless you have permission to create WX
> memory or to at least create W memory and then change it to X.
> 
> b) You can do whatever you want with /dev/xyz, and LSM policies are
> blatantly violated as a result.

I think the important issue at hand is that classic LSM policies
simply are not relevant with respect to how this technology was
designed to operate, and perhaps more importantly, how people want to
use it.

That is why I have consistently stated that I think the only relevant
knob is a binary decision as to whether or not a platform owner wants
to entertain completely anonymous code execution.

> c) The /dev/xyz API is clever and tracks, page-by-page, whether the
> user intends to ever write and/or execute that page, and behaves
> accordingly.
>
> This patchset takes the approach (c).  The actual clever policy
> isn't here yet, and we don't really know whether it will ever
> appear, but the API is set up to enable such a policy to be written.
> This appears to be a win for everyone, since the code is pretty
> clean and the API is straightforward.

I believe I have been clear in stating that I have never doubted the
cleverness of the approach or its potential utility for the future.

The issue at hand is that it simply isn't relevant at this stage of
the driver.  Getting this new vision right is something that would
benefit from a lot of conversations between runtime and kernel
developers.  Arguably, the case can be made that it should have a
second type of implementation to ensure that the approach is generic,
extensible and most importantly secure.

The 'cleverness' of the policy needs to be evaluated in the context of
what does it mean with respect to the risk arbitration decisions that
we are trying to support.  The open question comes down to, in
essence, asking ourselves whether or not we believe that it makes
sense to say that 15 pages of RWX memory is a security threat but 5
are not.

> Now, back to SGX.  There are only two things that are remotely
> SGX-specific here.  First, SGX requires this unusual memory model in
> which there is an executable mapping of (part of) a device node. [0]
> Second, early SGX hardware had this oddity that the kernel could set
> a per-backing-page (as opposed to per-PTE) bit to permanently
> disable X on a given /dev/sgx page.  Building a security model
> around that would have been a hack, and it DOES NOT WORK on new
> hardware.  So can we please stop discussing it?  None of the actual
> interesting parts of this have much to do with SGX per se and have
> nothing whatsoever to do with EDMM or any other Intel buzzword.

Just a clarification for everyone sitting in their recliners eating
popcorn and following along at home.

As I've stated before, I don't argue the potential utility of some new
model, SGX however, has hardware characteristics that cannot be waved
away in this discussion.  The technology was designed to have a
cryptographic measurement that controls whether or not the memory
image is suitable for execution.  The description of that image is
defined by the enclave author when the enclave is signed.

This is why the current EDMM implementation requires that a maximum
aperture range be defined for dynamic memory regions.  Since the
linear address of each page address in the enclave is encoded into the
measurement, enclave initialization will fail unless the loaded memory
image is consistent with the wishes of the enclave signer.  Having 40
pages of potential heap memory when the author called for 39 would be
considered an initialization defect that would be enforced by the
hardware.

The desired page permissions are also encoded into the enclave
measurement.  Since the current implementation takes the maximum
scoped permissions from the security information encoded in the
enclave, it would require that the enclave encode for RWX permissions
if the intent was to dynmically load executable or JIT code after the
enclave was initialized.  If I understand your above analysis
correctly, this would be problematic for current security
models/practices.

Obviously an API could be proposed that allowed this permissable
memory map to be defined independently from the enclave.  This notion,
based on my read of the security risk considerations that went into
the design of SGX, would be problematic, since it would allow an
untrusted party to modify the characteristics that were desired by the
enclave author for the executable image.

> Heck, if anyone actually cared to do so, something with essentially
> the same semantics could probably be built using SEV hardware
> instead of SGX, and it would have exactly the same issue if we
> wanted it to work for tasks that didn't have access to /dev/kvm.

As I noted above, perhaps whatever this driver may become in the
future would benefit from the community creating something like this
as a second reference to build an API on top of.

> [0] SGX doesn't *really* require this.  We could set things up so that
> you do mmap(..., MAP_ANONYMOUS, fd, ...) and then somehow introduce
> that mapping to SGX.  I think the result would be too disgusting to
> seriously consider.

Let me be clear, I certainly would not advocate doing anything too
disgusting to consider.

Hopefully our proposal for simplifying the security model for the
driver, while still allowing the framework for a still unspecified
future pathway, doesn't fit this description.

Best wishes for a productive week to everyone.

Dr. Greg

As always,
Dr. Greg Wettstein, Ph.D, Worker      Autonomously self-defensive
Enjellic Systems Development, LLC     IOT platforms and edge devices.
4206 N. 19th Ave.
Fargo, ND  58102
PH: 701-281-1686                      EMAIL: greg@enjellic.com
------------------------------------------------------------------------------
"Boy, it must not take much to make a phone work.  Looking at
 everthing else here it must be the same way with the INTERNET."
                                -- Francis 'Fritz' Wettstein

  reply	other threads:[~2020-11-16 18:01 UTC|newest]

Thread overview: 75+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-04 14:54 [PATCH v40 00/24] Intel SGX foundations Jarkko Sakkinen
2020-11-04 14:54 ` [PATCH v40 01/24] x86/sgx: Add SGX architectural data structures Jarkko Sakkinen
2020-11-04 14:54 ` [PATCH v40 02/24] x86/sgx: Add wrappers for ENCLS functions Jarkko Sakkinen
2020-11-04 14:54 ` [PATCH v40 03/24] x86/sgx: Initialize metadata for Enclave Page Cache (EPC) sections Jarkko Sakkinen
2020-11-04 18:21   ` Borislav Petkov
2020-11-04 19:04     ` Jarkko Sakkinen
2020-11-04 19:09       ` Jarkko Sakkinen
2020-11-04 19:12         ` Borislav Petkov
2020-11-04 14:54 ` [PATCH v40 04/24] x86/cpufeatures: x86/msr: Add Intel SGX hardware bits Jarkko Sakkinen
2020-11-04 14:54 ` [PATCH v40 05/24] x86/cpufeatures: x86/msr: Add Intel SGX Launch Control " Jarkko Sakkinen
2020-11-04 14:54 ` [PATCH v40 06/24] x86/mm: x86/sgx: Signal SIGSEGV with PF_SGX Jarkko Sakkinen
2020-11-04 14:54 ` [PATCH v40 07/24] x86/cpu/intel: Detect SGX support Jarkko Sakkinen
2020-11-04 14:54 ` [PATCH v40 08/24] x86/cpu/intel: Add nosgx kernel parameter Jarkko Sakkinen
2020-11-04 14:54 ` [PATCH v40 09/24] x86/sgx: Add SGX page allocator functions Jarkko Sakkinen
2020-11-05 15:08   ` Borislav Petkov
2020-11-04 14:54 ` [PATCH v40 10/24] mm: Add 'mprotect' hook to struct vm_operations_struct Jarkko Sakkinen
2020-11-05 16:04   ` Borislav Petkov
2020-11-05 17:33     ` Dave Hansen
2020-11-06 10:04   ` Mel Gorman
2020-11-06 16:51     ` Jarkko Sakkinen
2020-11-06 20:37       ` Borislav Petkov
2020-11-06 22:04         ` Jarkko Sakkinen
2020-11-06 22:31           ` Borislav Petkov
2020-11-06 17:43   ` Dr. Greg
2020-11-06 17:54     ` Dave Hansen
2020-11-07 15:09       ` Dr. Greg
2020-11-07 19:16         ` Dave Hansen
2020-11-12 20:58           ` Dr. Greg
2020-11-12 21:31             ` Dave Hansen
2020-11-12 22:41               ` Andy Lutomirski
2020-11-12 22:41                 ` Andy Lutomirski
2020-11-16 18:00                 ` Dr. Greg [this message]
2020-11-19  1:39                   ` Haitao Huang
2020-11-20 17:31                     ` Dr. Greg
2020-11-15 18:59               ` Dr. Greg
2020-11-06 21:13     ` Matthew Wilcox
2020-11-06 21:23       ` Dave Hansen
2020-11-07 15:27       ` Dr. Greg
2020-11-04 14:54 ` [PATCH v40 11/24] x86/sgx: Add SGX misc driver interface Jarkko Sakkinen
2020-11-05  1:10   ` Jarkko Sakkinen
2020-11-05  1:16     ` Jarkko Sakkinen
2020-11-05 16:05       ` Borislav Petkov
2020-11-05 17:57         ` Jarkko Sakkinen
2020-11-05 18:10           ` Borislav Petkov
2020-11-06 16:07             ` Jarkko Sakkinen
2020-11-06 17:09               ` Borislav Petkov
2020-11-06 22:01                 ` Jarkko Sakkinen
2020-11-04 14:54 ` [PATCH v40 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE Jarkko Sakkinen
2020-11-04 14:54 ` [PATCH v40 13/24] x86/sgx: Add SGX_IOC_ENCLAVE_ADD_PAGES Jarkko Sakkinen
2020-11-04 14:54 ` [PATCH v40 14/24] x86/sgx: Add SGX_IOC_ENCLAVE_INIT Jarkko Sakkinen
2020-11-04 14:54 ` [PATCH v40 15/24] x86/sgx: Add SGX_IOC_ENCLAVE_PROVISION Jarkko Sakkinen
2020-11-04 14:54 ` [PATCH v40 16/24] x86/vdso: Add support for exception fixup in vDSO functions Jarkko Sakkinen
2020-11-04 14:54 ` [PATCH v40 17/24] x86/fault: Add helper function to sanitize error code Jarkko Sakkinen
2020-11-04 14:54 ` [PATCH v40 18/24] x86/traps: Attempt to fixup exceptions in vDSO before signaling Jarkko Sakkinen
2020-11-04 14:54 ` [PATCH v40 19/24] x86/vdso: Implement a vDSO for Intel SGX enclave call Jarkko Sakkinen
2020-11-08 18:24   ` Jethro Beekman
2020-11-08 20:08   ` Jethro Beekman
2020-11-08 20:26     ` Dave Hansen
2020-11-08 20:20   ` Jethro Beekman
2020-11-08 20:30     ` Dave Hansen
2020-11-04 14:54 ` [PATCH v40 20/24] selftests/x86: Add a selftest for SGX Jarkko Sakkinen
2020-11-04 14:54 ` [PATCH v40 21/24] x86/sgx: Add a page reclaimer Jarkko Sakkinen
2020-11-08  3:56   ` Hillf Danton
2020-11-09 19:59     ` Jarkko Sakkinen
2020-11-04 14:54 ` [PATCH v40 22/24] x86/sgx: Add ptrace() support for the SGX driver Jarkko Sakkinen
2020-11-04 14:54 ` [PATCH v40 23/24] docs: x86/sgx: Document SGX kernel architecture Jarkko Sakkinen
2020-11-04 14:54 ` [PATCH v40 24/24] x86/sgx: Update MAINTAINERS Jarkko Sakkinen
2020-11-08 20:48 ` [PATCH v40 00/24] Intel SGX foundations Jethro Beekman
2020-11-21 15:12 ` Dr. Greg
2020-11-21 16:25   ` Dave Hansen
2020-11-24 10:55     ` Dr. Greg
2020-11-24 17:49       ` Andy Lutomirski
2020-11-21 18:36   ` Andy Lutomirski
2020-11-24 18:40     ` Dr. Greg
2020-11-24 21:57       ` Andy Lutomirski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201116180023.GA32481@wind.enjellic.com \
    --to=greg@enjellic.com \
    --cc=akpm@linux-foundation.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=asapek@google.com \
    --cc=bp@alien8.de \
    --cc=cedric.xing@intel.com \
    --cc=chenalexchen@google.com \
    --cc=conradparker@google.com \
    --cc=cyhanish@google.com \
    --cc=darren.kenny@oracle.com \
    --cc=dave.hansen@intel.com \
    --cc=haitao.huang@intel.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=jethro@fortanix.com \
    --cc=kai.huang@intel.com \
    --cc=kai.svahn@intel.com \
    --cc=kmoy@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-sgx@vger.kernel.org \
    --cc=ludloff@google.com \
    --cc=luto@kernel.org \
    --cc=mikko.ylinen@intel.com \
    --cc=nhorman@redhat.com \
    --cc=npmccallum@redhat.com \
    --cc=puiterwijk@redhat.com \
    --cc=rientjes@google.com \
    --cc=sean.j.christopherson@intel.com \
    --cc=tglx@linutronix.de \
    --cc=willy@infradead.org \
    --cc=x86@kernel.org \
    --cc=yaozhangx@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.