Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 27 May 2020 09:49:59 +0100
From:   Stefan Hajnoczi <stefanha@gmail.com>
To:     Andra Paraschiv <andraprs@amazon.com>
Cc:     linux-kernel@vger.kernel.org,
        Anthony Liguori <aliguori@amazon.com>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Colm MacCarthaigh <colmmacc@amazon.com>,
        Bjoern Doebel <doebel@amazon.de>,
        David Woodhouse <dwmw@amazon.co.uk>,
        Frank van der Linden <fllinden@amazon.com>,
        Alexander Graf <graf@amazon.de>,
        Martin Pohlack <mpohlack@amazon.de>,
        Matt Wilson <msw@amazon.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Balbir Singh <sblbir@amazon.com>,
        Stefano Garzarella <sgarzare@redhat.com>,
        Stefan Hajnoczi <stefanha@redhat.com>,
        Stewart Smith <trawets@amazon.com>,
        Uwe Dannowski <uwed@amazon.de>, kvm@vger.kernel.org,
        ne-devel-upstream@amazon.com
Subject: Re: [PATCH v3 01/18] nitro_enclaves: Add ioctl interface definition
Message-ID: <20200527084959.GA29137@stefanha-x1.localdomain>
References: <20200525221334.62966-1-andraprs@amazon.com>
 <20200525221334.62966-2-andraprs@amazon.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="vkogqOf2sHV7VnPd"
Content-Disposition: inline
In-Reply-To: <20200525221334.62966-2-andraprs@amazon.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--vkogqOf2sHV7VnPd
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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.
>=20
> Include part of the KVM ioctls in the provided ioctl interface, with
> additional NE ioctl commands that e.g. triggers the enclave run.
>=20
> Signed-off-by: Alexandru Vasile <lexnv@amazon.com>
> Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
> ---
> Changelog
>=20
> v2 -> v3
>=20
> * Remove the GPL additional wording as SPDX-License-Identifier is already=
 in
> place.
>=20
> v1 -> v2
>=20
> * Add ioctl for getting enclave image load metadata.
> * Update NE_ENCLAVE_START ioctl name to NE_START_ENCLAVE.=20
> * 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
>=20
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documen=
tation/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                                                             Net=
filter device in development:
>                                                                       <ma=
ilto:rusty@rustcorp.com.au>
> -0xAE  all    linux/kvm.h                                             Ker=
nel-based Virtual Machine
> +0xAE  00-1F  linux/kvm.h                                             Ker=
nel-based Virtual Machine
>                                                                       <ma=
ilto:kvm@vger.kernel.org>
> +0xAE  40-FF  linux/kvm.h                                             Ker=
nel-based Virtual Machine
> +                                                                     <ma=
ilto:kvm@vger.kernel.org>
> +0xAE  20-3F  linux/nitro_enclaves.h                                  Nit=
ro Enclaves
>  0xAF  00-1F  linux/fsl_hypervisor.h                                  Fre=
escale hypervisor
>  0xB0  all                                                            RAT=
IO devices in development:
>                                                                       <ma=
ilto: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_enclave=
s.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 Reserve=
d.
> + */
> +
> +#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/nit=
ro_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 Reserve=
d.
> + */
> +
> +#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 i=
mage
> + * loading e.g. offset in enclave memory to start placing the enclave im=
age.
> + *
> + * The image load metadata is an in / out data structure. It includes in=
fo
> + * provided by the caller - flags - and returns the offset in enclave me=
mory
> + * where to start placing the enclave image.
> + */
> +#define NE_GET_IMAGE_LOAD_METADATA _IOWR(0xAE, 0x20, struct image_load_m=
etadata)
> +
> +/**
> + * The command is used to trigger enclave start after the enclave resour=
ces,
> + * 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.

--vkogqOf2sHV7VnPd
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEhpWov9P5fNqsNXdanKSrs4Grc8gFAl7OKbcACgkQnKSrs4Gr
c8jJVAgAo7KasfyNLaEi6MJPaDQp8v3M/aDIOEGPQsQ7f9cGH63De1A98S9ZOa/s
derpLcBDuMKFpTbU8QOUI4ihiiFRQR6KYRRsmvNkxWDi7bdF9NpNILhvN991gLQT
ui6GfLtCSuYkhdYS5gxg6AQ2kEHT6eGqA/lmXDnfe5pmPg0rOZV+QhhSxq7i6THO
UpnqPRAch2uA2TMRZfx6HX7tK+yUlPqlzS3lrxmzVVA3ixZY5uHgsAX0IvKLkUna
T056VDXxl+B1O3GmxpfXEyw+TQee5nJG8D3xYs7+QTeIa9YDyfua3C0UsufxGZiT
jv0DHwuj5R3uGbJTNwJ/hYeu5+9exg==
=nwyo
-----END PGP SIGNATURE-----

--vkogqOf2sHV7VnPd--