Messages in this thread | | | Date | Tue, 10 Oct 2023 22:06:43 +0200 | Subject | Re: [PATCH v5] misc: Add Nitro Secure Module driver | From | Christophe JAILLET <> |
| |
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
> +}
...
| |