Messages in this thread | | | Subject | Re: [PATCH v3 01/18] nitro_enclaves: Add ioctl interface definition | From | "Paraschiv, Andra-Irina" <> | Date | Thu, 28 May 2020 21:05:12 +0300 |
| |
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.
| |