Re: [PATCH v5] misc: Add Nitro Secure Module driver

[Alexander Graf <graf@amazon.com>]

Hi Christophe,

Thanks a bunch for the review!


On 10.10.23 22:06, Christophe JAILLET wrote:
>
>
> Le 10/10/2023 à 21:18, Alexander Graf a écrit :
>> When running Linux inside a Nitro Enclave, the hypervisor provides a
>> special virtio device called "Nitro Security Module" (NSM). This device
>> has 3 main functions:
>>
>>    1) Provide attestation reports
>>    2) Modify PCR state
>>    3) Provide entropy
>>
>> This patch adds a driver for NSM that exposes a /dev/nsm device node 
>> which
>> user space can issue an ioctl on this device with raw NSM CBOR formatted
>> commands to request attestation documents, influence PCR states, read
>> entropy and enumerate status of the device. In addition, the driver
>> implements a hwrng backend.
>>
>> Originally-by: Petre Eftime <petre.eftime@gmail.com>
>> Signed-off-by: Alexander Graf <graf@amazon.com>
>>
>> ---
>
> ...
>
>> +static int nsm_rng_read(struct hwrng *rng, void *data, size_t max, 
>> bool wait)
>> +{
>> +     struct nsm *nsm = hwrng_to_nsm(rng);
>> +     struct device *dev = &nsm->vdev->dev;
>> +     struct nsm_msg *msg;
>> +     int rc = 0;
>> +
>> +     /* NSM always needs to wait for a response */
>> +     if (!wait)
>> +             return 0;
>> +
>> +     msg = devm_kzalloc(dev, sizeof(*msg), GFP_KERNEL);
>> +     if (!msg)
>> +             return -ENOMEM;
>
> Why use devm_ here? It is freed in all cases...


Yes, absolutely. Let me change that :)


>
>> +
>> +     rc = fill_req_get_random(nsm, &msg->req);
>> +     if (rc != 0)
>> +             goto out;
>> +
>> +     rc = nsm_sendrecv_msg(nsm, msg);
>> +     if (rc != 0)
>> +             goto out;
>> +
>> +     rc = parse_resp_get_random(nsm, &msg->resp, data, max);
>> +     if (rc < 0)
>> +             goto out;
>> +
>> +     dev_dbg(dev, "RNG: returning rand bytes = %d", rc);
>> +out:
>> +     devm_kfree(dev, msg);
>
> ..., so kfree would be just fine as well.
>
>> +     return rc;
>> +}
>> +
>> +static long nsm_dev_ioctl(struct file *file, unsigned int cmd,
>> +     unsigned long arg)
>> +{
>> +     void __user *argp = u64_to_user_ptr((u64)arg);
>> +     struct nsm *nsm = file_to_nsm(file);
>> +     struct device *dev = &nsm->vdev->dev;
>> +     struct nsm_msg *msg;
>> +     struct nsm_raw raw;
>> +     int r = 0;
>> +
>> +     if (cmd != NSM_IOCTL_RAW)
>> +             return -EINVAL;
>> +
>> +     if (_IOC_SIZE(cmd) != sizeof(raw))
>> +             return -EINVAL;
>> +
>> +     /* Allocate message buffers to device */
>> +     r = -ENOMEM;
>> +     msg = devm_kzalloc(dev, sizeof(*msg), GFP_KERNEL);
>
> Why use devm_ here? It is freed in all cases...
>
>> +     if (!msg)
>> +             goto out;
>> +
>> +     /* Copy user argument struct to kernel argument struct */
>> +     r = -EFAULT;
>> +     if (copy_from_user(&raw, argp, _IOC_SIZE(cmd)))
>> +             goto out;
>> +
>> +     /* Convert kernel argument struct to device request */
>> +     r = fill_req_raw(nsm, &msg->req, &raw);
>> +     if (r)
>> +             goto out;
>> +
>> +     /* Send message to NSM and read reply */
>> +     r = nsm_sendrecv_msg(nsm, msg);
>> +     if (r)
>> +             goto out;
>> +
>> +     /* Parse device response into kernel argument struct */
>> +     r = parse_resp_raw(nsm, &msg->resp, &raw);
>> +     if (r)
>> +             goto out;
>> +
>> +     /* Copy kernel argument struct back to user argument struct */
>> +     r = -EFAULT;
>> +     if (copy_to_user(argp, &raw, sizeof(raw)))
>> +             goto out;
>> +
>> +     r = 0;
>> +
>> +out:
>> +     devm_kfree(dev, msg);
>
> ..., so kfree would be just fine as well.
>
>> +     return r;
>> +}
>
> ...
>
>> +/* Handler for probing the NSM device */
>> +static int nsm_device_probe(struct virtio_device *vdev)
>> +{
>> +     struct device *dev = &vdev->dev;
>> +     struct nsm *nsm;
>> +     int rc;
>> +
>> +     nsm = devm_kzalloc(&vdev->dev, sizeof(*nsm), GFP_KERNEL);
>> +     if (!nsm)
>> +             return -ENOMEM;
>> +
>> +     vdev->priv = nsm;
>> +     nsm->vdev = vdev;
>> +
>> +     rc = nsm_device_init_vq(vdev);
>> +     if (rc) {
>> +             dev_err(dev, "queue failed to initialize: %d.\n", rc);
>> +             goto err_init_vq;
>> +     }
>> +
>> +     mutex_init(&nsm->lock);
>> +     init_waitqueue_head(&nsm->wq);
>> +
>> +     /* Register as hwrng provider */
>> +     nsm->hwrng = (struct hwrng) {
>> +             .read = nsm_rng_read,
>> +             .name = "nsm-hwrng",
>> +             .quality = 1000,
>> +     };
>> +
>> +     rc = devm_hwrng_register(&vdev->dev, &nsm->hwrng);
>> +     if (rc) {
>> +             dev_err(dev, "RNG initialization error: %d.\n", rc);
>> +             goto err_hwrng;
>> +     }
>> +
>> +     /* Register /dev/nsm device node */
>> +     nsm->misc = (struct miscdevice) {
>> +             .minor  = MISC_DYNAMIC_MINOR,
>> +             .name   = "nsm",
>> +             .fops   = &nsm_dev_fops,
>> +             .mode   = 0666,
>> +     };
>> +
>> +     rc = misc_register(&nsm->misc);
>> +     if (rc) {
>> +             dev_err(dev, "misc device registration error: %d.\n", rc);
>> +             goto err_misc;
>> +     }
>> +
>> +     return 0;
>> +
>> +err_misc:
>> +     hwrng_unregister(&nsm->hwrng);
>
> Is it needed? devm_hwrng_register() is used.


I'm afraid I can't / shouldn't use devm_hwrng_register() here. I need to 
clean up the vq pointer in the remove function which I think may race 
during removal. I really don't want to have hwrng call the driver code 
while we've already destroyed the vqs.

So I'll change it to non-devm calls.


>
>> +err_hwrng:
>> +     vdev->config->del_vqs(vdev);
>> +err_init_vq:
>> +     kfree(nsm);
>
> 'nsm' is devm_kzalloc()'ed, so this is a double free.


Thanks for catching this!


>
>> +     return rc;
>> +}
>> +
>> +/* Handler for removing the NSM device */
>> +static void nsm_device_remove(struct virtio_device *vdev)
>> +{
>> +     struct nsm *nsm = vdev->priv;
>> +
>> +     hwrng_unregister(&nsm->hwrng);
>
> Is it needed? devm_hwrng_register() is used.
>
>> +
>> +     vdev->config->del_vqs(vdev);
>> +     misc_deregister(&nsm->misc);
>> +     list_del(&nsm->node);
>
> When is it list_add()'ed?
> The 'node' field seems unused.


This is a leftover from old code, removed. Thanks for catching it!


Alex




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