Re: [PATCH v40 00/24] Intel SGX foundations

From: Andy Lutomirski <luto@kernel.org>
To: "Dr. Greg" <greg@enjellic.com>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	X86 ML <x86@kernel.org>,
	linux-sgx@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	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>,
	"Huang, Kai" <kai.huang@intel.com>,
	"Svahn, Kai" <kai.svahn@intel.com>, Keith Moyer <kmoy@google.com>,
	Christian Ludloff <ludloff@google.com>,
	Andrew Lutomirski <luto@kernel.org>,
	Neil Horman <nhorman@redhat.com>,
	Nathaniel McCallum <npmccallum@redhat.com>,
	Patrick Uiterwijk <puiterwijk@redhat.com>,
	David Rientjes <rientjes@google.com>,
	"Christopherson, Sean J" <sean.j.christopherson@intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	yaozhangx@google.com, Mikko Ylinen <mikko.ylinen@intel.com>
Subject: Re: [PATCH v40 00/24] Intel SGX foundations
Date: Sat, 21 Nov 2020 10:36:58 -0800	[thread overview]
Message-ID: <CALCETrXTjZSg-ccDKF7yX8y+vCCrcn9C_m6ZCGNdWHq73aUHQQ@mail.gmail.com> (raw)
In-Reply-To: <20201121151259.GA3948@wind.enjellic.com>

Dr. Greg, I know you like sending these emails, but they're not really
helpful for Linux kernel development.  Please see below.

On Sat, Nov 21, 2020 at 7:13 AM Dr. Greg <greg@enjellic.com> wrote:
>
> On Wed, Nov 04, 2020 at 04:54:06PM +0200, Jarkko Sakkinen wrote:
>
> Good morning, I hope the weekend is going well for everyone.
>
> > Important Kernel Touch Points
> > =============================
> >
> > This implementation is picky and will decline to work on hardware which
> > is locked to Intel's root of trust.
>
> Given that this driver is no longer locked to the Intel trust root, by
> virtue of being restricted to run only on platforms which support
> Flexible Launch Control, there is no longer any legitimate technical
> reason to not expose all of the functionality of the hardware.

I read this three times, and I can't tell what functionality you're
referring to.

>
> The patch that I am including below implements signature based policy
> controls for enclave initialization.  It does so in a manner that is
> completely transparent to the default behavior of the driver, which is
> to initialize any enclave passed to it with the exception of enclaves
> that set the PROVISION_KEY attribute bit.

It's completely unreviewable.  It's an ABI patch, and it does not
document what it does, nor does it document why it does it.  It's just
a bunch of code.  NAK.

To be crystal clear, I will not review, and I will NAK outright, any
patches of this sort, until ALL of the following conditions are met:

a) Either a functioning SGX driver lands in a -rc kernel or there is
an excellent justification for why a change of this sort is needed
prior to a release.  Dr. Greg, you seem to be interested in SGX
actually landing upstream, but these patches are just causing delays.
Please stop.

b) The patch needs to explain what problem it solves and why it is a
good solution to that problem.

c) The patch needs to explain, either in a changelog or in a text file
in the patch, *exactly* what it does.  Imagine MSDN-like documentation
in the good old days.  The documentation needs to be sufficiently
clear that a userspace programmer could use your mechanism without
reference to your implementation and that a kernel programmer could,
in principle, re-implement your code from the description.

Unless all three of these are met, your patch is going nowhere, and I
think no one should waste their time trying to read it.

>
> Secondary to the discussions that have been ongoing regarding the
> restriction of mmap/mprotect, this patch has been extended to
> implement signature based controls on dynamic enclaves.  The default
> behavior of the driver under this patch is to reject mmap/mprotect on
> initialized enclaves, unless the platform owner has elected to allow
> the enclave signer the option to implement 'dynamic' enclaves,
> ie. enclaves that are allowed to modify their page permissions after
> initialization.

You have sent this change repeatedly, and now for some reason you're
sending it mixed in with unrelated changes.  Please stop.  At no point
have you explained why this approach is better than anything else.  It
has the obnoxious side effect that you can't usefully SCM_RIGHTS an
enclave to a different process with your patch applied, which is at
least a minor disadvantage.  You have not explained any advantage to
your patch at all.

Dr Greg, before you hit send on further emails about SGX, could you
kindly try to imagine you're a kernel maintainer, read your own email,
and consider whether how to make it add something useful to the
discussion?

Thanks,
Andy