On Fri, Sep 29, 2023, at 09:33, Alexander Graf wrote:
When running Linux inside a Nitro Enclave, the hypervisor provides aHi Alex,
special virtio device called "NSM". This device has 2 main functions:
1) Provide attestation reports
2) Modify PCR state
3) Provide entropy
This patch adds the core NSM driver that exposes a /dev/nsm device node
which user space can use to request attestation documents and influence
PCR states. A follow up patch will add a hwrng driver to feed its entropy
into the kernel.
Originally-by: Petre Eftime <petre.eftime@xxxxxxxxx>
Signed-off-by: Alexander Graf <graf@xxxxxxxxxx>
I've taken a first look at this driver and have some minor comments.
The main point here is that I think we need to look at possible
alternatives for the user space interface, and (if possible) change
to a set of higher-level ioctl commands from the simple passthrough.
+/* Virtio MMIO device definition */
+struct virtio_mmio_device {
+ struct virtio_device vdev;
+ struct platform_device *pdev;
+
+ void __iomem *base;
+ unsigned long version;
+
+ /* a list of queues so we can dispatch IRQs */
+ spinlock_t lock;
+ struct list_head virtqueues;
+};
+
+/* Virtqueue list entry */
+struct virtio_mmio_vq_info {
+ /* The actual virtqueue */
+ struct virtqueue *vq;
+
+ /* The list node for the virtqueues list */
+ struct list_head node;
+};
It looks like you are duplicating these structures from the
virtio_mmio.c file, which seems like a bad idea for a number
of reasons. What is it that you actually need that the
virtio subsystem does not provide? Can you add interfaces
to the common code instead?
+static struct virtio_device *nsm_vdev;Instead of global structures, these should ideally all be
+static struct nsm_hwrng *nsm_hwrng;
+static struct mutex nsm_lock;
+static wait_queue_head_t nsm_waitqueue;
+static bool nsm_device_notified;
part of a per-device structure, even if you are sure that
there is only ever one of these devices.
+/* Copy an entire message from user-space to kernel-space */It looks like the ioctl interface just provides an interface
+static int message_memdup_from_user(struct nsm_kernel_message *dst,
+ struct nsm_message *src)
+{
+ struct nsm_message shallow_copy;
+
+ if (!src || !dst)
+ return -EINVAL;
+
+ /* The destination's request and response buffers should be NULL. */
+ if (dst->request.iov_base || dst->response.iov_base)
+ return -EINVAL;
+
+ /* First, make a shallow copy to be able to read the inner pointers */
+ if (copy_from_user(&shallow_copy, src, sizeof(shallow_copy)) != 0)
+ return -EINVAL;
+
+ /* Verify the user input size. */
+ if (shallow_copy.request.iov_len > NSM_REQUEST_MAX_SIZE)
+ return -EMSGSIZE;
+
+ /* Allocate kernel memory for the user request */
+ dst->request.iov_len = shallow_copy.request.iov_len;
+ dst->request.iov_base = kmalloc(dst->request.iov_len, GFP_KERNEL);
+ if (!dst->request.iov_base)
+ return -ENOMEM;
+
+ /* Copy the request content */
+ if (copy_from_user(dst->request.iov_base,
+ shallow_copy.request.iov_base, dst->request.iov_len) != 0) {
+ kfree(dst->request.iov_base);
+ return -EFAULT;
+ }
for passing through raw messages, which is often not the best
idea. Are you able to enumerate the possible request types and
provide a separate ioctl for each one?
+/* Supported driver operations */This breaks on 32-bit userspace, which would need a separate .compat_ioctl
+static const struct file_operations nsm_dev_fops = {
+ .open = nsm_dev_file_open,
+ .release = nsm_dev_file_close,
+ .unlocked_ioctl = nsm_dev_ioctl,
+};
handler with the current command definition. It's often better to define
the ioctl interface to be the same on 32-bit and 64-bit userspace
and then use the trivial compat_ptr_ioctl wrapper.
+/* Driver configuration */I would suggest expanding NSM_DEV_NAME here, it's much easier to
+static struct miscdevice nsm_driver_miscdevice = {
+ .minor = MISC_DYNAMIC_MINOR,
+ .name = NSM_DEV_NAME,
+ .fops = &nsm_dev_fops,
+ .mode = 0666
+};
grep for the actual string if a user wants to know which driver
is responsible. Probably even less code.
+ if (nsm_hwrng)The debug statements can probably get removed, especially
+ nsm_hwrng->probe(vdev);
+
+ pr_debug("NSM device has been probed.\n");
+ return 0;
+}
the whitespace damaged ones.
+int nsm_register_hwrng(struct nsm_hwrng *_nsm_hwrng)This should get easier of you reverse the dependency between
+{
+ if (nsm_hwrng)
+ return -EEXIST;
+
+ nsm_hwrng = _nsm_hwrng;
+ if (nsm_vdev)
+ nsm_hwrng->probe(nsm_vdev);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(nsm_register_hwrng);
the two drivers and just call into the nsm_hwrng_probe()
function from the main driver's probe.
+ mutex_init(&nsm_lock);You can simply use DEFINE_MUTEX() and DECLARE_WAIT_QUEUE_HEAD()
+ init_waitqueue_head(&nsm_waitqueue);
if you still need the global objects (rather than making them
per device).
+Then this can use module_virtio_driver()
+ rc = register_virtio_driver(&virtio_nsm_driver);
+ if (rc)
+ pr_err("NSM driver initialization error: %d.\n", rc);
+
+ return rc;
+}
+
+static void __exit nsm_driver_exit(void)
+{
+ unregister_virtio_driver(&virtio_nsm_driver);
+ mutex_destroy(&nsm_lock);
+ pr_debug("NSM driver exited.\n");
+}
+
+module_init(nsm_driver_init);
+module_exit(nsm_driver_exit);
Arnd