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@xxxxxxxxx>
Signed-off-by: Alexander Graf <graf@xxxxxxxxxx>
---
...
+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
