lkml.org 
[lkml]   [2023]   [Oct]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v5] misc: Add Nitro Secure Module driver
From
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...

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

> +err_hwrng:
> + vdev->config->del_vqs(vdev);
> +err_init_vq:
> + kfree(nsm);

'nsm' is devm_kzalloc()'ed, so this is a double free.

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

CJ

> +}

...

\
 
 \ /
  Last update: 2023-10-10 22:09    [W:0.061 / U:0.360 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site