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

From: Stefan Hajnoczi
Date: Wed May 27 2020 - 04:50:05 EST

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@xxxxxxxxxx>
> Signed-off-by: Andra Paraschiv <andraprs@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
> -0xAE all linux/kvm.h Kernel-based Virtual Machine
> +0xAE 00-1F linux/kvm.h Kernel-based Virtual Machine
> <mailto:kvm@xxxxxxxxxxxxxxx>
> +0xAE 40-FF linux/kvm.h Kernel-based Virtual Machine
> + <mailto:kvm@xxxxxxxxxxxxxxx>
> +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@xxxxxxxx>

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.

> 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.

> +/* 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).

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.

Attachment: signature.asc
Description: PGP signature