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

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6489 bytes --]

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.

> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]