Re: [PATCH v19 00/27] Intel SGX1 support

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

On Tue, Mar 19, 2019 at 04:41:03PM -0700, Sean Christopherson wrote:
> On Sun, Mar 17, 2019 at 11:14:29PM +0200, Jarkko Sakkinen wrote:
> > 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.
> > 
> > There is a new hardware unit in the processor called Memory Encryption
> > Engine (MEE) starting from the Skylake microacrhitecture. BIOS can define
> > one or many MEE regions that can hold enclave data by configuring them with
> > PRMRR registers.
> > 
> > The MEE automatically encrypts the data leaving the processor package to
> > the MEE regions. The data is encrypted using a random key whose life-time
> > is exactly one power cycle.
> > 
> > The current implementation requires that the firmware sets
> > IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can
> > decide what enclaves it wants run. The implementation does not create
> > any bottlenecks to support read-only MSRs later on.
> > 
> > You can tell if your CPU supports SGX by looking into /proc/cpuinfo:
> > 
> > 	cat /proc/cpuinfo  | grep sgx
> > 
> > v19:
> 
> ...
> 
> > * Allow the driver to be compiled as a module now that it no code is using
> >   its routines and it only uses exported symbols. Now the driver is
> >   essentially just a thin ioctl layer.
> 
> I'm not convinced it's worth the effort to allow the driver to be compiled
> as a module, especially if we drop the ACPI-based probing.  Making the
> driver loadable means the kernel can easily end up in situations where it's
> tracking EPC and running its reclaimer kthread, but the driver can't be
> loaded and can *never* be loaded, e.g. because the platform doesn't support
> Launch Control.
> 
> And IMO encl.{c,h} belongs in the "driver" code, but to let the driver be
> loadable it got shoved into the core subsystem.  All of that code is
> specific to running enclaves in the host, i.e. it shouldn't exist if I
> compile out the driver entirely (in a future world where I want the core
> SGX subsystem for virtualization purposes).
> 
> And yes, I realize this is a full 180 from my stance a year ago :)

I'm perfectly fine with removing platform driver is that is the common
opinion. I would still keep the bus (or equivalent) thought because it
gives possibility in the future add sysfs attributes.

> 
> > * Allow forking i.e. remove VM_DONTCOPY. I did not change the API
> >   because the old API scaled to the workload that Andy described. The
> >   codebase is now mostly API independent i.e. changing the API is a
> >   small task. For me the proper trigger to chanage it is a as concrete
> >   as possible workload that cannot be fulfilled. I hope you understand
> >   my thinking here. I don't want to change anything w/o proper basis
> >   but I'm ready to change anything if there is a proper basis. I do
> >   not have any kind of attachment to any particular type of API.
> 
> It's not just forking, it's being able to hand off an enclave to an
> already running process.  Andy also had (IMO) valid complaints about
> completely ignoring @filep in the ioctls() and searching the vma to
> find the enclave, e.g. the current code has to acquire mmap_sem just
> to get a reference to the enclave and a process has to mmap() the
> enclave to use any ioctl() other than ECREATE.

I'm cool with this and internals are now in a shape that is trivial
to implement. Just would want an example of workload where that
would be useful. It is not only for decision making but also
for reflecting whether the change is done correctly.

We should probably also extend the selftest to do some trivial forking
or SCM_RIGHTS kind of thing depending on the API.

/Jarkko