Date:   Tue, 28 Apr 2020 17:07:34 +0200
From:   Alexander Graf <graf@...zon.com>
To:     Paolo Bonzini <pbonzini@...hat.com>,
        "Paraschiv, Andra-Irina" <andraprs@...zon.com>,
        <linux-kernel@...r.kernel.org>
CC:     Anthony Liguori <aliguori@...zon.com>,
        Benjamin Herrenschmidt <benh@...zon.com>,
        Colm MacCarthaigh <colmmacc@...zon.com>,
        Bjoern Doebel <doebel@...zon.de>,
        David Woodhouse <dwmw@...zon.co.uk>,
        Frank van der Linden <fllinden@...zon.com>,
        Martin Pohlack <mpohlack@...zon.de>,
        Matt Wilson <msw@...zon.com>, Balbir Singh <sblbir@...zon.com>,
        Stewart Smith <trawets@...zon.com>,
        Uwe Dannowski <uwed@...zon.de>, <kvm@...r.kernel.org>,
        <ne-devel-upstream@...zon.com>
Subject: Re: [PATCH v1 00/15] Add support for Nitro Enclaves



On 25.04.20 18:05, Paolo Bonzini wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 24/04/20 21:11, Alexander Graf wrote:
>> What I was saying above is that maybe code is easier to transfer that
>> than a .txt file that gets lost somewhere in the Documentation directory
>> :).
> 
> whynotboth.jpg :D

Uh, sure? :)

Let's first hammer out what we really want for the UABI though. Then we 
can document it.

>>>> To answer the question though, the target file is in a newly invented
>>>> file format called "EIF" and it needs to be loaded at offset 0x800000 of
>>>> the address space donated to the enclave.
>>>
>>> What is this EIF?
>>
>> It's just a very dumb container format that has a trivial header, a
>> section with the bzImage and one to many sections of initramfs.
>>
>> As mentioned earlier in this thread, it really is just "-kernel" and
>> "-initrd", packed into a single binary for transmission to the host.
> 
> Okay, got it.  So, correct me if this is wrong, the information that is
> needed to boot the enclave is:
> 
> * the kernel, in bzImage format
> 
> * the initrd

It's a single EIF file for a good reason. There are checksums in there 
and potentially signatures too, so that you can the enclave can attest 
itself. For the sake of the user space API, the enclave image really 
should just be considered a blob.

> 
> * a consecutive amount of memory, to be mapped with
> KVM_SET_USER_MEMORY_REGION
> 
> Off list, Alex and I discussed having a struct that points to kernel and
> initrd off enclave memory, and have the driver build EIF at the
> appropriate point in enclave memory (the 8 MiB ofset that you mentioned).
> 
> This however has two disadvantages:
> 
> 1) having the kernel and initrd loaded by the parent VM in enclave
> memory has the advantage that you save memory outside the enclave memory
> for something that is only needed inside the enclave
> 
> 2) it is less extensible (what if you want to use PVH in the future for
> example) and puts in the driver policy that should be in userspace.
> 
> 
> So why not just start running the enclave at 0xfffffff0 in real mode?
> Yes everybody hates it, but that's what OSes are written against.  In
> the simplest example, the parent enclave can load bzImage and initrd at
> 0x10000 and place firmware tables (MPTable and DMI) somewhere at
> 0xf0000; the firmware would just be a few movs to segment registers
> followed by a long jmp.

There is a bit of initial attestation flow in the enclave, so that you 
can be sure that the code that is running is actually what you wanted to 
run.

I would also in general prefer to disconnect the notion of "enclave 
memory" as much as possible from a memory location view. User space 
shouldn't be in the business of knowing location of its donated memory 
ended up at which enclave memory position. By disconnecting the view of 
the memory world, we can do some more optimizations, such as compact 
memory ranges more efficiently in kernel space.

> If you want to keep EIF, we measured in QEMU that there is no measurable
> difference between loading the kernel in the host and doing it in the
> guest, so Amazon could provide an EIF loader stub at 0xfffffff0 for
> backwards compatibility.

It's not about performance :).

So the other thing we discussed was whether the KVM API really turned 
out to be a good fit here. After all, today we merely call:

   * CREATE_VM
   * SET_MEMORY_RANGE
   * CREATE_VCPU
   * START_ENCLAVE

where we even butcher up CREATE_VCPU into a meaningless blob of overhead 
for no good reason.

Why don't we build something like the following instead?

   vm = ne_create(vcpus = 4)
   ne_set_memory(vm, hva, len)
   ne_load_image(vm, addr, len)
   ne_start(vm)

That way we would get the EIF loading into kernel space. "LOAD_IMAGE" 
would only be available in the time window between set_memory and start. 
It basically implements a memcpy(), but it would completely hide the 
hidden semantics of where an EIF has to go, so future device versions 
(or even other enclave implementers) could change the logic.

I think it also makes sense to just allocate those 4 ioctls from 
scratch. Paolo, would you still want to "donate" KVM ioctl space in that 
case?

Overall, the above should address most of the concerns you raised in 
this mail, right? It still requires copying, but at least we don't have 
to keep the copy in kernel space.


Alex



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Powered by blists - more mailing lists