Re: [PATCH v2 1/2] misc: Add Nitro Secure Module driver

[Alexander Graf]

Hi Arnd!

On 29.09.23 19:28, Arnd Bergmann wrote:
> On Fri, Sep 29, 2023, at 09:33, Alexander Graf wrote:
> > When running Linux inside a Nitro Enclave, the hypervisor provides a
> > 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>
> Hi Alex,
>
> I've taken a first look at this driver and have some minor comments.


Thanks a bunch!


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


I'm slightly torn on that bit. I think in hindsight the NSM device 
probably should have been a reserved vsock CID and the hwrng one should 
have just been virtio-rng.

The problem is that Nitro Enclaves were launched in 2020 and since an 
ecosystem developed in multiple languages to support building code inside:

https://github.com/aws/aws-nitro-enclaves-nsm-api/blob/main/src/driver/mod.rs#L66
https://github.com/donkersgoed/aws-nsm-interface/blob/main/aws_nsm_interface/__init__.py#L264-L274
  https://github.com/hf/nsm/blob/main/nsm.go#L99-L129


All of these use the (downstream) ioctl that this patch also implements. 
We could change it, but instead of making it easier for user space to 
adapt the device node, it would probably hurt more.

I agree that this is not a great place to be in. This driver absolutely 
should have been upstreamed 3 years ago. But I can't turn back time 
(yet) :).

> > +/* 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?


Thanks for catching this. There are proper interfaces to get the virt 
queues already, let me use them instead.


> > +static struct virtio_device *nsm_vdev;
> > +static struct nsm_hwrng *nsm_hwrng;
> > +static struct mutex nsm_lock;
> > +static wait_queue_head_t nsm_waitqueue;
> > +static bool nsm_device_notified;
> Instead of global structures, these should ideally all be
> part of a per-device structure, even if you are sure that
> there is only ever one of these devices.


Let me give that a try :)


> > +/* Copy an entire message from user-space to kernel-space */
> > +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;
> > +     }
> It looks like the ioctl interface just provides an interface
> 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?


See above. I could, but I think it would not improve the situation. It 
would also require a significant amount of CBOR parsing in the kernel 
which I'd rather avoid :).


> > +/* Supported driver operations */
> > +static const struct file_operations nsm_dev_fops = {
> > +     .open = nsm_dev_file_open,
> > +     .release = nsm_dev_file_close,
> > +     .unlocked_ioctl = nsm_dev_ioctl,
> > +};
> This breaks on 32-bit userspace, which would need a separate .compat_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 */
> > +static struct miscdevice nsm_driver_miscdevice = {
> > +     .minor  = MISC_DYNAMIC_MINOR,
> > +     .name   = NSM_DEV_NAME,
> > +     .fops   = &nsm_dev_fops,
> > +     .mode   = 0666
> > +};
> I would suggest expanding NSM_DEV_NAME here, it's much easier to
> grep for the actual string if a user wants to know which driver
> is responsible. Probably even less code.
>
> > +     if (nsm_hwrng)
> > +             nsm_hwrng->probe(vdev);
> > +
> > +     pr_debug("NSM device has been probed.\n");
> > +     return 0;
> > +}
> The debug statements can probably get removed, especially
> the whitespace damaged ones.
>
> > +int nsm_register_hwrng(struct nsm_hwrng *_nsm_hwrng)
> > +{
> > +     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);
> This should get easier of you reverse the dependency between
> the two drivers and just call into the nsm_hwrng_probe()
> function from the main driver's probe.


I don't understand what you mean by reversing the dependency. Nsm_rng is 
a downstream of Nsm, because Nsm is the virtio device that owns the channel.


> > +     mutex_init(&nsm_lock);
> > +     init_waitqueue_head(&nsm_waitqueue);
> You can simply use DEFINE_MUTEX() and DECLARE_WAIT_QUEUE_HEAD()
> if you still need the global objects (rather than making them
> per device).
>
> > +
> > +     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);
> Then this can use module_virtio_driver()


Definitely!

Thanks :). I'll have some typing to do.


Alex


>        Arnd



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