Message-ID: <CALCETrXSFh2v9WZe9c1sT3w6di7uvk4j2fk6bGdb6YT4=rrHdQ@mail.gmail.com>
Date:   Thu, 18 Apr 2019 11:07:17 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     "Dr. Greg" <greg@...ellic.com>
Cc:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>,
        linux-sgx@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Dave Hansen <dave.hansen@...el.com>,
        "Christopherson, Sean J" <sean.j.christopherson@...el.com>,
        nhorman@...hat.com, npmccallum@...hat.com,
        "Ayoun, Serge" <serge.ayoun@...el.com>,
        "Katz-zamir, Shay" <shay.katz-zamir@...el.com>,
        "Huang, Haitao" <haitao.huang@...el.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Svahn, Kai" <kai.svahn@...el.com>, Borislav Petkov <bp@...en8.de>,
        Josh Triplett <josh@...htriplett.org>,
        Andrew Lutomirski <luto@...nel.org>,
        "Huang, Kai" <kai.huang@...el.com>,
        David Rientjes <rientjes@...gle.com>
Subject: Re: [PATCH v20 00/28] Intel SGX1 support

On Thu, Apr 18, 2019 at 10:11 AM Dr. Greg <greg@...ellic.com> wrote:
>
> On Wed, Apr 17, 2019 at 01:39:10PM +0300, Jarkko Sakkinen wrote:
>
> Good morning, I hope this note finds the impending end of the work
> week going well for everyone.
>
> First of all, a thank you to Jarkko for his efforts in advancing this
> driver, a process that can be decidedly thankless.  Our apologies in
> advance for contributing to this phenomenon.
>
> > Intel(R) SGX is a set of CPU instructions that can be used by
> > applications to set aside private regions of code and data. The code
> > outside the enclave is disallowed to access the memory inside the
> > enclave by the CPU access control.  In a way you can think that SGX
> > provides inverted sandbox. It protects the application from a
> > malicious host.
>
> Unfortunately, in its current implementation, the driver will not
> protect a host from malicious enclaves.  If it advances in its current
> form, the mainline Linux kernel will be implementing a driver with
> known and documented security issues.

"I can use this kernel or CPU feature to do something unpleasant that
I could do anyway, if less nicely" is a far cry from "known and
documented security issues".

We've hashed this out to death.  Once SGX is upstream, feel free to
submit patches.

>
> In addition, the driver breaks all existing SGX software by breaking
> compatibility with what is a 3+ year ABI provided by the existing
> driver.  This seems to contravene the well understood philosophy that
> Linux doesn't, if at all possible, break existing applications,
> something that we believe can be easily prevented if those interested
> in these issues choose to read on.

As I understand it, Cedric is going to submit a patch for this
shortly, assuming I have correctly guessed what supposed ABI break
you're talking about.

In the mean time, I need to correct you on a critical point.  Linux
has *never* had a policy that it will retain compatibility with an API
that wasn't upstream in the first place.

[removed extensive verbiage.  Dr. Greg, if you want your emails to be
read, please try to be more concise, and please try to repeat yourself
less.]

> The attached patch, against the jarkko-sgx/master branch of Jarkko's
> driver repository, provides a framework that implements
> cryptographically secure enclave initialization policies.  The patch
> and it's signature are also available from the following URL's:
>
> ftp://ftp.idfusion.net/pub/idfusion/jarkko-master-SFLC.patch

I just spend several minutes trying to read your code.  It would
benefit dramatically from some attempt at documentation, and, at the
very least, you can't have a function foo(type *ptr) that does
something almost completely different when ptr == NULL and when ptr !=
NULL.  For a concrete example, consider sgx_launch_control().  If you
pass NULL, then there's a vaguely credible argument that the function
does what the name suggests.  If you pass non-NULL, it doesn't.  The
result is bug-prone and unreadable.

If I'm understanding it right, it causes the SGX ABI to be almost
completely incompatible between the case where the list of launch
signers is empty and the case where the list is non-empty.  This is a
non-starter.  We're also extensively discussed how launch enclaves
would be supported if we were to support them, and the generally
agreed-upon solution was that the *kernel* would handle running a
launch enclave.  Jarkko even has code for this, but it's tabled until
someone comes up with a credible argument that we *want* to support
launch enclaves.

> The policy architecture allows just about any conceivable
> initialization policy to be implemented and in any combination.

No it doesn't, because it requires the application (or its runtime or
SDK or whatever) to bundle the launch enclave that implements the
fancy policy, which means that the platform owner actually can't swap
out the policy without breaking all the applications.  This is the
primary reason that, if Linux were to support LE-based launch control,
it would do so in the kernel.

> If the platform owner chooses to do nothing, the driver will
> initialize any enclave that is presented to it.  FWIW, we have close
> to a century of enterprise system's administration experience on our
> team and we could not envision any competent system's administrator,
> concerned about security, who would allow such a configuration.

A sufficiently careful administrator who wants to protect against
enclave malware might want the ability to revoke permission to run a
specific enclave, which your approach can't do.  Again, this has all
been covered extensively.

To be crystal clear, i fully agree that we are likely to eventually
want some kind of in-kernel launch policy.  But I don't think that
defining such a policy should block the upstreaming of the driver, and
I don't think that your proposal for how the policy should work is an
acceptable proposal.

Powered by blists - more mailing lists