Re: [PATCH v3 01/18] nitro_enclaves: Add ioctl interface definition

["Paraschiv, Andra-Irina" <andraprs@amazon.com>]

On 27/05/2020 11:49, Stefan Hajnoczi wrote:
> On Tue, May 26, 2020 at 01:13:17AM +0300, Andra Paraschiv wrote:
>> The Nitro Enclaves driver handles the enclave lifetime management. This
>> includes enclave creation, termination and setting up its resources such
>> as memory and CPU.
>>
>> An enclave runs alongside the VM that spawned it. It is abstracted as a
>> process running in the VM that launched it. The process interacts with
>> the NE driver, that exposes an ioctl interface for creating an enclave
>> and setting up its resources.
>>
>> Include part of the KVM ioctls in the provided ioctl interface, with
>> additional NE ioctl commands that e.g. triggers the enclave run.
>>
>> Signed-off-by: Alexandru Vasile <lexnv@amazon.com>
>> Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
>> ---
>> Changelog
>>
>> v2 -> v3
>>
>> * Remove the GPL additional wording as SPDX-License-Identifier is already in
>> place.
>>
>> v1 -> v2
>>
>> * Add ioctl for getting enclave image load metadata.
>> * Update NE_ENCLAVE_START ioctl name to NE_START_ENCLAVE.
>> * Add entry in Documentation/userspace-api/ioctl/ioctl-number.rst for NE ioctls.
>> * Update NE ioctls definition based on the updated ioctl range for major and
>> minor.
>> ---
>>   .../userspace-api/ioctl/ioctl-number.rst      |  5 +-
>>   include/linux/nitro_enclaves.h                | 11 ++++
>>   include/uapi/linux/nitro_enclaves.h           | 65 +++++++++++++++++++
>>   3 files changed, 80 insertions(+), 1 deletion(-)
>>   create mode 100644 include/linux/nitro_enclaves.h
>>   create mode 100644 include/uapi/linux/nitro_enclaves.h
>>
>> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
>> index f759edafd938..8a19b5e871d3 100644
>> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
>> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
>> @@ -325,8 +325,11 @@ Code  Seq#    Include File                                           Comments
>>   0xAC  00-1F  linux/raw.h
>>   0xAD  00                                                             Netfilter device in development:
>>                                                                        <mailto:rusty@rustcorp.com.au>
>> -0xAE  all    linux/kvm.h                                             Kernel-based Virtual Machine
>> +0xAE  00-1F  linux/kvm.h                                             Kernel-based Virtual Machine
>>                                                                        <mailto:kvm@vger.kernel.org>
>> +0xAE  40-FF  linux/kvm.h                                             Kernel-based Virtual Machine
>> +                                                                     <mailto:kvm@vger.kernel.org>
>> +0xAE  20-3F  linux/nitro_enclaves.h                                  Nitro Enclaves
>>   0xAF  00-1F  linux/fsl_hypervisor.h                                  Freescale hypervisor
>>   0xB0  all                                                            RATIO devices in development:
>>                                                                        <mailto:vgo@ratio.de>
> Reusing KVM ioctls seems a little hacky. Even the ioctls that are used
> by this driver don't use all the fields or behave in the same way as
> kvm.ko.
>
> For example, the memory regions slot number is not used by Nitro
> Enclaves.
>
> It would be cleaner to define NE-specific ioctls instead.

Indeed, a couple of fields / parameters are not used during the KVM 
ioctl calls for now.

Will adapt the logic to follow a similar model of creating VMs and 
adding resources, with NE ioctls.

>
>> diff --git a/include/linux/nitro_enclaves.h b/include/linux/nitro_enclaves.h
>> new file mode 100644
>> index 000000000000..d91ef2bfdf47
>> --- /dev/null
>> +++ b/include/linux/nitro_enclaves.h
>> @@ -0,0 +1,11 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved.
>> + */
>> +
>> +#ifndef _LINUX_NITRO_ENCLAVES_H_
>> +#define _LINUX_NITRO_ENCLAVES_H_
>> +
>> +#include <uapi/linux/nitro_enclaves.h>
>> +
>> +#endif /* _LINUX_NITRO_ENCLAVES_H_ */
>> diff --git a/include/uapi/linux/nitro_enclaves.h b/include/uapi/linux/nitro_enclaves.h
>> new file mode 100644
>> index 000000000000..3413352baf32
>> --- /dev/null
>> +++ b/include/uapi/linux/nitro_enclaves.h
>> @@ -0,0 +1,65 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +/*
>> + * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved.
>> + */
>> +
>> +#ifndef _UAPI_LINUX_NITRO_ENCLAVES_H_
>> +#define _UAPI_LINUX_NITRO_ENCLAVES_H_
>> +
>> +#include <linux/kvm.h>
>> +#include <linux/types.h>
>> +
>> +/* Nitro Enclaves (NE) Kernel Driver Interface */
>> +
>> +/**
>> + * The command is used to get information needed for in-memory enclave image
>> + * loading e.g. offset in enclave memory to start placing the enclave image.
>> + *
>> + * The image load metadata is an in / out data structure. It includes info
>> + * provided by the caller - flags - and returns the offset in enclave memory
>> + * where to start placing the enclave image.
>> + */
>> +#define NE_GET_IMAGE_LOAD_METADATA _IOWR(0xAE, 0x20, struct image_load_metadata)
>> +
>> +/**
>> + * The command is used to trigger enclave start after the enclave resources,
>> + * such as memory and CPU, have been set.
>> + *
>> + * The enclave start metadata is an in / out data structure. It includes info
>> + * provided by the caller - enclave cid and flags - and returns the slot uid
>> + * and the cid (if input cid is 0).
>> + */
>> +#define NE_START_ENCLAVE _IOWR(0xAE, 0x21, struct enclave_start_metadata)
>> +
> image_load_metadata->flags and enclave_start_metadata->flags constants
> are missing.

I added them now in this file. Thank you.

>
>> +/* Metadata necessary for in-memory enclave image loading. */
>> +struct image_load_metadata {
>> +	/**
>> +	 * Flags to determine the enclave image type e.g. Enclave Image Format
>> +	 * (EIF) (in).
>> +	 */
>> +	__u64 flags;
>> +
>> +	/**
>> +	 * Offset in enclave memory where to start placing the enclave image
>> +	 * (out).
>> +	 */
>> +	__u64 memory_offset;
>> +};
> What about feature bits or a API version number field? If you add
> features to the NE driver, how will userspace detect them?
>
> Even if you intend to always compile userspace against the exact kernel
> headers that the program will run on, it can still be useful to have an
> API version for informational purposes and to easily prevent user
> errors (running a new userspace binary on an old kernel where the API is
> different).

Good point. I'll add a get API version ioctl for this purpose.

>
> Finally, reserved struct fields may come in handy in the future. That
> way userspace and the kernel don't need to explicitly handle multiple
> struct sizes.

True, I've seen this pattern of including reserved fields e.g. in a 
couple of KVM data structures.

Thanks for feedback, Stefan.

Andra



Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.