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

[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@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

> +}

...