Re: [PATCH v3 3/5] device/virtio-nsm: Support for Nitro Secure Module device

["Daniel P. Berrangé" <berrange@redhat.com>]

On Sat, Aug 10, 2024 at 10:45:00PM +0600, Dorjoy Chowdhury wrote:
> Nitro Secure Module (NSM)[1] device is used in AWS Nitro Enclaves for
> stripped down TPM functionality like cryptographic attestation. The
> requests to and responses from NSM device are CBOR[2] encoded.
> 
> This commit adds support for NSM device in QEMU. Although related to
> AWS Nitro Enclaves, the virito-nsm device is independent and can be
> used in other machine types as well. The libcbor[3] library has been
> used for the CBOR encoding and decoding functionalities.
> 
> [1] https://lists.oasis-open.org/archives/virtio-comment/202310/msg00387.html
> [2] http://cbor.io/
> [3] https://libcbor.readthedocs.io/en/latest/
> 
> Signed-off-by: Dorjoy Chowdhury <dorjoychy111@gmail.com>
> ---
>  MAINTAINERS                    |    8 +
>  hw/virtio/Kconfig              |    5 +
>  hw/virtio/meson.build          |    4 +
>  hw/virtio/virtio-nsm-pci.c     |   73 ++
>  hw/virtio/virtio-nsm.c         | 1929 ++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio-nsm.h |   59 +
>  6 files changed, 2078 insertions(+)
>  create mode 100644 hw/virtio/virtio-nsm-pci.c
>  create mode 100644 hw/virtio/virtio-nsm.c
>  create mode 100644 include/hw/virtio/virtio-nsm.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f8d63031f0..05b66a7f93 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2349,6 +2349,14 @@ F: include/sysemu/rng*.h
>  F: backends/rng*.c
>  F: tests/qtest/virtio-rng-test.c
>  
> +virtio-nsm
> +M: Alexander Graf <graf@amazon.com>
> +M: Dorjoy Chowdhury <dorjoychy111@gmail.com>
> +S: Maintained
> +F: hw/virtio/virtio-nsm.c
> +F: hw/virtio/virtio-nsm-pci.c
> +F: include/hw/virtio/virtio-nsm.h
> +
>  vhost-user-stubs
>  M: Alex Bennée <alex.bennee@linaro.org>
>  S: Maintained
> diff --git a/hw/virtio/Kconfig b/hw/virtio/Kconfig
> index aa63ff7fd4..29fee32035 100644
> --- a/hw/virtio/Kconfig
> +++ b/hw/virtio/Kconfig
> @@ -6,6 +6,11 @@ config VIRTIO_RNG
>      default y
>      depends on VIRTIO
>  
> +config VIRTIO_NSM
> +   bool
> +   default y
> +   depends on VIRTIO
> +
>  config VIRTIO_IOMMU
>      bool
>      default y
> diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
> index 621fc65454..831819eefb 100644
> --- a/hw/virtio/meson.build
> +++ b/hw/virtio/meson.build
> @@ -48,12 +48,15 @@ else
>    system_virtio_ss.add(files('vhost-stub.c'))
>  endif
>  
> +libcbor = dependency('libcbor', version: '>=0.8.0')
> +
>  specific_virtio_ss.add(when: 'CONFIG_VIRTIO_BALLOON', if_true: files('virtio-balloon.c'))
>  specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true: files('vhost-user-fs.c'))
>  specific_virtio_ss.add(when: 'CONFIG_VIRTIO_PMEM', if_true: files('virtio-pmem.c'))
>  specific_virtio_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true: files('vhost-vsock.c'))
>  specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_VSOCK', if_true: files('vhost-user-vsock.c'))
>  specific_virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-rng.c'))
> +specific_virtio_ss.add(when: 'CONFIG_VIRTIO_NSM', if_true: [files('virtio-nsm.c'), libcbor])
>  specific_virtio_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: files('virtio-mem.c'))
>  specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_SCMI', if_true: files('vhost-user-scmi.c'))
>  specific_virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_SCMI'], if_true: files('vhost-user-scmi-pci.c'))
> @@ -70,6 +73,7 @@ virtio_pci_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true: files('virtio-crypto-pc
>  virtio_pci_ss.add(when: 'CONFIG_VIRTIO_INPUT_HOST', if_true: files('virtio-input-host-pci.c'))
>  virtio_pci_ss.add(when: 'CONFIG_VIRTIO_INPUT', if_true: files('virtio-input-pci.c'))
>  virtio_pci_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-rng-pci.c'))
> +virtio_pci_ss.add(when: 'CONFIG_VIRTIO_NSM', if_true: [files('virtio-nsm-pci.c'), libcbor])
>  virtio_pci_ss.add(when: 'CONFIG_VIRTIO_BALLOON', if_true: files('virtio-balloon-pci.c'))
>  virtio_pci_ss.add(when: 'CONFIG_VIRTIO_9P', if_true: files('virtio-9p-pci.c'))
>  virtio_pci_ss.add(when: 'CONFIG_VIRTIO_SCSI', if_true: files('virtio-scsi-pci.c'))
> diff --git a/hw/virtio/virtio-nsm-pci.c b/hw/virtio/virtio-nsm-pci.c
> new file mode 100644
> index 0000000000..dca797315a
> --- /dev/null
> +++ b/hw/virtio/virtio-nsm-pci.c
> @@ -0,0 +1,73 @@
> +/*
> + * AWS Nitro Secure Module (NSM) device
> + *
> + * Copyright (c) 2024 Dorjoy Chowdhury <dorjoychy111@gmail.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * (at your option) any later version.  See the COPYING file in the
> + * top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "hw/virtio/virtio-pci.h"
> +#include "hw/virtio/virtio-nsm.h"
> +#include "hw/qdev-properties.h"
> +#include "qapi/error.h"
> +#include "qemu/module.h"
> +#include "qom/object.h"
> +
> +typedef struct VirtIONsmPCI VirtIONsmPCI;
> +
> +#define TYPE_VIRTIO_NSM_PCI "virtio-nsm-pci-base"
> +DECLARE_INSTANCE_CHECKER(VirtIONsmPCI, VIRTIO_NSM_PCI,
> +                         TYPE_VIRTIO_NSM_PCI)
> +
> +struct VirtIONsmPCI {
> +    VirtIOPCIProxy parent_obj;
> +    VirtIONSM vdev;
> +};
> +
> +static void virtio_nsm_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> +{
> +    VirtIONsmPCI *vnsm = VIRTIO_NSM_PCI(vpci_dev);
> +    DeviceState *vdev = DEVICE(&vnsm->vdev);
> +
> +    virtio_pci_force_virtio_1(vpci_dev);
> +
> +    if (!qdev_realize(vdev, BUS(&vpci_dev->bus), errp)) {
> +        return;
> +    }
> +}
> +
> +static void virtio_nsm_pci_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
> +
> +    k->realize = virtio_nsm_pci_realize;
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +}
> +
> +static void virtio_nsm_initfn(Object *obj)
> +{
> +    VirtIONsmPCI *dev = VIRTIO_NSM_PCI(obj);
> +
> +    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
> +                                TYPE_VIRTIO_NSM);
> +}
> +
> +static const VirtioPCIDeviceTypeInfo virtio_nsm_pci_info = {
> +    .base_name             = TYPE_VIRTIO_NSM_PCI,
> +    .generic_name          = "virtio-nsm-pci",
> +    .instance_size = sizeof(VirtIONsmPCI),
> +    .instance_init = virtio_nsm_initfn,
> +    .class_init    = virtio_nsm_pci_class_init,
> +};
> +
> +static void virtio_nsm_pci_register(void)
> +{
> +    virtio_pci_types_register(&virtio_nsm_pci_info);
> +}
> +
> +type_init(virtio_nsm_pci_register)
> diff --git a/hw/virtio/virtio-nsm.c b/hw/virtio/virtio-nsm.c
> new file mode 100644
> index 0000000000..1610bcedc6
> --- /dev/null
> +++ b/hw/virtio/virtio-nsm.c
> @@ -0,0 +1,1929 @@
> +/*
> + * AWS Nitro Secure Module (NSM) device
> + *
> + * Copyright (c) 2024 Dorjoy Chowdhury <dorjoychy111@gmail.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * (at your option) any later version.  See the COPYING file in the
> + * top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/guest-random.h"
> +#include "qapi/error.h"
> +#include <cbor.h>
> +
> +#include "hw/virtio/virtio.h"
> +#include "hw/virtio/virtio-nsm.h"
> +#include "standard-headers/linux/virtio_ids.h"
> +
> +#define NSM_PCR_DATA_REQ_MAX_SIZE 512
> +
> +enum NSMResponseTypes {
> +    NSM_SUCCESS = 0,
> +    NSM_INVALID_ARGUMENT = 1,
> +    NSM_INVALID_INDEX = 2,
> +    NSM_READONLY_INDEX = 3,
> +    NSM_INVALID_OPERATION = 4,
> +    NSM_BUFFER_TOO_SMALL = 5,
> +    NSM_INPUT_TOO_LARGE = 6,
> +    NSM_INTERNAL_ERROR = 7,
> +};
> +
> +static bool qemu_cbor_map_add(cbor_item_t *map, cbor_item_t *key,
> +                              cbor_item_t *value)
> +{
> +    bool success = false;
> +    struct cbor_pair pair = (struct cbor_pair) {
> +        .key = cbor_move(key),
> +        .value = cbor_move(value)
> +    };
> +
> +    success = cbor_map_add(map, pair);
> +    if (!success) {
> +        cbor_incref(pair.key);
> +        cbor_incref(pair.value);
> +    }
> +
> +    return success;
> +}

I'd suggest all these 'qemu_cbor_' helper methods could be split off into a
separate file, since this virtio-nsm.c is pretty large with all these helpers
inlines.

> +static bool extend_pcr(VirtIONSM *vnsm, int ind, uint8_t *data, uint16_t len)
> +{
> +    GChecksum *hasher = g_checksum_new(G_CHECKSUM_SHA384);
> +    struct PCRInfo *pcr = &(vnsm->pcrs[ind]);
> +    size_t digest_len = SHA384_BYTE_LEN;
> +    if (!hasher) {
> +        return false;
> +    }
> +
> +    g_checksum_update(hasher, pcr->data, SHA384_BYTE_LEN);
> +    g_checksum_update(hasher, data, len);
> +
> +    g_checksum_get_digest(hasher, pcr->data, &digest_len);
> +
> +    g_checksum_free(hasher);
> +    return true;
> +}

Use the qcrypto hash APIs instead of GChecksum please.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|