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