Re: [PATCH v4 12/18] nitro_enclaves: Add logic for enclave start

["Paraschiv, Andra-Irina" <andraprs@amazon.com>]

On 06/07/2020 14:21, Alexander Graf wrote:
>
>
> 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@amazon.com>
>> Signed-off-by: Andra Paraschiv <andraprs@amazon.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,
>   };

Good point. In v5, I moved a couple of sanity checks from the ioctl 
switch case block in this function, so this would not apply wrt the 
updated codebase. But I'll keep this suggestion as reference for other 
cases.

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

Done.

>
>> +
>> +            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?

Yes, it should be and this would help with bubbling up to user space the 
reason of failure in more detail.

I started to define a set of NE error codes and update the failure paths 
(e.g. this one and the others mentioned below) to use those error codes.

>
>> +        }
>> +
>> +        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?
>
>

Included "__user" in all the copy_from_user() / copy_to_user() calls.

Thank you.

Andra

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