| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 6 Jul 2020 13:21:49 +0200
From: Alexander Graf <graf@...zon.de>
To: Andra Paraschiv <andraprs@...zon.com>,
<linux-kernel@...r.kernel.org>
CC: Anthony Liguori <aliguori@...zon.com>,
Benjamin Herrenschmidt <benh@...nel.crashing.org>,
Colm MacCarthaigh <colmmacc@...zon.com>,
"Bjoern Doebel" <doebel@...zon.de>,
David Woodhouse <dwmw@...zon.co.uk>,
"Frank van der Linden" <fllinden@...zon.com>,
Greg KH <gregkh@...uxfoundation.org>,
Martin Pohlack <mpohlack@...zon.de>,
Matt Wilson <msw@...zon.com>,
"Paolo Bonzini" <pbonzini@...hat.com>,
Balbir Singh <sblbir@...zon.com>,
"Stefano Garzarella" <sgarzare@...hat.com>,
Stefan Hajnoczi <stefanha@...hat.com>,
Stewart Smith <trawets@...zon.com>,
Uwe Dannowski <uwed@...zon.de>, <kvm@...r.kernel.org>,
<ne-devel-upstream@...zon.com>
Subject: Re: [PATCH v4 12/18] nitro_enclaves: Add logic for enclave start
On 22.06.20 22:03, Andra Paraschiv wrote:
> After all the enclave resources are set, the enclave is ready for
> beginning to run.
>
> Add ioctl command logic for starting an enclave after all its resources,
> memory regions and CPUs, have been set.
>
> The enclave start information includes the local channel addressing -
> vsock CID - and the flags associated with the enclave.
>
> Signed-off-by: Alexandru Vasile <lexnv@...zon.com>
> Signed-off-by: Andra Paraschiv <andraprs@...zon.com>
> ---
> Changelog
>
> v3 -> v4
>
> * Use dev_err instead of custom NE log pattern.
> * Update the naming for the ioctl command from metadata to info.
> * Check for minimum enclave memory size.
>
> v2 -> v3
>
> * Remove the WARN_ON calls.
> * Update static calls sanity checks.
>
> v1 -> v2
>
> * Add log pattern for NE.
> * Check if enclave state is init when starting an enclave.
> * Remove the BUG_ON calls.
> ---
> drivers/virt/nitro_enclaves/ne_misc_dev.c | 114 ++++++++++++++++++++++
> 1 file changed, 114 insertions(+)
>
> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> index 17ccb6cdbd75..d9794f327169 100644
> --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> @@ -703,6 +703,45 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave,
> return rc;
> }
>
> +/**
> + * ne_start_enclave_ioctl - Trigger enclave start after the enclave resources,
> + * such as memory and CPU, have been set.
> + *
> + * This function gets called with the ne_enclave mutex held.
> + *
> + * @ne_enclave: private data associated with the current enclave.
> + * @enclave_start_info: enclave info that includes enclave cid and flags.
> + *
> + * @returns: 0 on success, negative return value on failure.
> + */
> +static int ne_start_enclave_ioctl(struct ne_enclave *ne_enclave,
> + struct ne_enclave_start_info *enclave_start_info)
> +{
> + struct ne_pci_dev_cmd_reply cmd_reply = {};
> + struct enclave_start_req enclave_start_req = {};
> + int rc = -EINVAL;
> +
> + enclave_start_req.enclave_cid = enclave_start_info->enclave_cid;
> + enclave_start_req.flags = enclave_start_info->flags;
> + enclave_start_req.slot_uid = ne_enclave->slot_uid;
I think it's easier to read if you do the initialization straight in the
variable declaation:
struct enclave_start_req enclave_start_req = {
.enclave_cid = enclave_start_info->cid,
.flags = enclave_start_info->flags,
.slot_uid = ne_enclave->slot_uid,
};
> +
> + rc = ne_do_request(ne_enclave->pdev, ENCLAVE_START, &enclave_start_req,
> + sizeof(enclave_start_req), &cmd_reply,
> + sizeof(cmd_reply));
> + if (rc < 0) {
> + dev_err_ratelimited(ne_misc_dev.this_device,
> + "Error in enclave start [rc=%d]\n", rc);
> +
> + return rc;
> + }
> +
> + ne_enclave->state = NE_STATE_RUNNING;
> +
> + enclave_start_info->enclave_cid = cmd_reply.enclave_cid;
> +
> + return 0;
> +}
> +
> static long ne_enclave_ioctl(struct file *file, unsigned int cmd,
> unsigned long arg)
> {
> @@ -818,6 +857,81 @@ static long ne_enclave_ioctl(struct file *file, unsigned int cmd,
> return rc;
> }
>
> + case NE_START_ENCLAVE: {
> + struct ne_enclave_start_info enclave_start_info = {};
> + int rc = -EINVAL;
> +
> + if (copy_from_user(&enclave_start_info, (void *)arg,
> + sizeof(enclave_start_info))) {
> + dev_err_ratelimited(ne_misc_dev.this_device,
> + "Error in copy from user\n");
No need to print anything here
> +
> + return -EFAULT;
> + }
> +
> + mutex_lock(&ne_enclave->enclave_info_mutex);
> +
> + if (ne_enclave->state != NE_STATE_INIT) {
> + dev_err_ratelimited(ne_misc_dev.this_device,
> + "Enclave isn't in init state\n");
> +
> + mutex_unlock(&ne_enclave->enclave_info_mutex);
> +
> + return -EINVAL;
Can this be its own return value instead?
> + }
> +
> + if (!ne_enclave->nr_mem_regions) {
> + dev_err_ratelimited(ne_misc_dev.this_device,
> + "Enclave has no mem regions\n");
> +
> + mutex_unlock(&ne_enclave->enclave_info_mutex);
> +
> + return -ENOMEM;
> + }
> +
> + if (ne_enclave->mem_size < NE_MIN_ENCLAVE_MEM_SIZE) {
> + dev_err_ratelimited(ne_misc_dev.this_device,
> + "Enclave memory is less than %ld\n",
> + NE_MIN_ENCLAVE_MEM_SIZE);
> +
> + mutex_unlock(&ne_enclave->enclave_info_mutex);
> +
> + return -ENOMEM;
> + }
> +
> + if (!ne_enclave->nr_vcpus) {
> + dev_err_ratelimited(ne_misc_dev.this_device,
> + "Enclave has no vcpus\n");
> +
> + mutex_unlock(&ne_enclave->enclave_info_mutex);
> +
> + return -EINVAL;
Same here.
> + }
> +
> + if (!cpumask_empty(ne_enclave->cpu_siblings)) {
> + dev_err_ratelimited(ne_misc_dev.this_device,
> + "CPU siblings not used\n");
> +
> + mutex_unlock(&ne_enclave->enclave_info_mutex);
> +
> + return -EINVAL;
Same here.
> + }
> +
> + rc = ne_start_enclave_ioctl(ne_enclave, &enclave_start_info);
> +
> + mutex_unlock(&ne_enclave->enclave_info_mutex);
> +
> + if (copy_to_user((void *)arg, &enclave_start_info,
This needs to be __user void *, no?
Alex
> + sizeof(enclave_start_info))) {
> + dev_err_ratelimited(ne_misc_dev.this_device,
> + "Error in copy to user\n");
> +
> + return -EFAULT;
> + }
> +
> + return rc;
> + }
> +
> default:
> return -ENOTTY;
> }
>
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
Powered by blists - more mailing lists