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

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

On Sun, Aug 18, 2024 at 05:42:54PM +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                      |   10 +
>  hw/virtio/Kconfig                |    5 +
>  hw/virtio/cbor-helpers.c         |  292 ++++++
>  hw/virtio/meson.build            |    4 +
>  hw/virtio/virtio-nsm-pci.c       |   73 ++
>  hw/virtio/virtio-nsm.c           | 1648 ++++++++++++++++++++++++++++++
>  include/hw/virtio/cbor-helpers.h |   43 +
>  include/hw/virtio/virtio-nsm.h   |   59 ++
>  8 files changed, 2134 insertions(+)
>  create mode 100644 hw/virtio/cbor-helpers.c
>  create mode 100644 hw/virtio/virtio-nsm-pci.c
>  create mode 100644 hw/virtio/virtio-nsm.c
>  create mode 100644 include/hw/virtio/cbor-helpers.h
>  create mode 100644 include/hw/virtio/virtio-nsm.h
> 

> diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
> index 621fc65454..7ccb61cf74 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.7.0')

In the following patch I suggest that we should have this at the top level
meson.build. Now that I see you're referencing this twice in different
places, we definitely want it in the top meson.build.

ALso the CI changes in Mention in the following patch should be in whatever
patch first introduces the libcbor dependency.



> diff --git a/hw/virtio/virtio-nsm.c b/hw/virtio/virtio-nsm.c
> new file mode 100644
> index 0000000000..e91848a2b0
> --- /dev/null
> +++ b/hw/virtio/virtio-nsm.c

> +static bool add_protected_header_to_cose(cbor_item_t *cose)
> +{
> +    cbor_item_t *map = NULL;
> +    cbor_item_t *key = NULL;
> +    cbor_item_t *value = NULL;
> +    cbor_item_t *bs = NULL;
> +    size_t len;
> +    bool r = false;
> +    size_t buf_len = 4096;
> +    uint8_t *buf = g_malloc(buf_len);

g_autofree avoids the manual  g_free call.

> +
> +    map = cbor_new_definite_map(1);
> +    if (!map) {
> +        goto cleanup;
> +    }
> +    key = cbor_build_uint8(1);
> +    if (!key) {
> +        goto cleanup;
> +    }
> +    value = cbor_new_int8();
> +    if (!value) {
> +        goto cleanup;
> +    }
> +    cbor_mark_negint(value);
> +    /* we don't actually sign the data, so we use -1 as the 'alg' value */
> +    cbor_set_uint8(value, 0);
> +
> +    if (!qemu_cbor_map_add(map, key, value)) {
> +        goto cleanup;
> +    }
> +
> +    len = cbor_serialize(map, buf, buf_len);
> +    if (len == 0) {
> +        goto cleanup_map;
> +    }
> +
> +    bs = cbor_build_bytestring(buf, len);
> +    if (!bs) {
> +        goto cleanup_map;
> +    }
> +    if (!qemu_cbor_array_push(cose, bs)) {
> +        cbor_decref(&bs);
> +        goto cleanup_map;
> +    }
> +    r = true;
> +    goto cleanup_map;
> +
> + cleanup:
> +    if (key) {
> +        cbor_decref(&key);
> +    }
> +    if (value) {
> +        cbor_decref(&value);
> +    }
> +
> + cleanup_map:
> +    if (map) {
> +        cbor_decref(&map);
> +    }
> +    g_free(buf);
> +    return r;
> +}
> +



> +static bool handle_Attestation(VirtIONSM *vnsm, struct iovec *request,
> +                               struct iovec *response, Error **errp)
> +{
> +    cbor_item_t *root = NULL;
> +    cbor_item_t *cose = NULL;
> +    cbor_item_t *nested_map;
> +    size_t len;
> +    NSMAttestationReq nsm_req;
> +    enum NSMResponseTypes type;
> +    bool r = false;
> +    size_t buf_len = 16384;
> +    uint8_t *buf = g_malloc(buf_len);

Another suitable for g_autofree

> +
> +    type = get_nsm_attestation_req(request->iov_base, request->iov_len,
> +                                   &nsm_req);
> +    if (type != NSM_SUCCESS) {
> +        if (error_response(response, type, errp)) {
> +            r = true;
> +        }
> +        goto out;
> +    }
> +
> +    cose = cbor_new_definite_array(4);
> +    if (!cose) {
> +        goto err;
> +    }
> +    if (!add_protected_header_to_cose(cose)) {
> +        goto err;
> +    }
> +    if (!add_unprotected_header_to_cose(cose)) {
> +        goto err;
> +    }
> +
> +    if (nsm_req.public_key_len > 0) {
> +        memcpy(vnsm->public_key, nsm_req.public_key, nsm_req.public_key_len);
> +        vnsm->public_key_len = nsm_req.public_key_len;
> +    }
> +    if (nsm_req.user_data_len > 0) {
> +        memcpy(vnsm->user_data, nsm_req.user_data, nsm_req.user_data_len);
> +        vnsm->user_data_len = nsm_req.user_data_len;
> +    }
> +    if (nsm_req.nonce_len > 0) {
> +        memcpy(vnsm->nonce, nsm_req.nonce, nsm_req.nonce_len);
> +        vnsm->nonce_len = nsm_req.nonce_len;
> +    }
> +
> +    if (!add_payload_to_cose(cose, vnsm)) {
> +        goto err;
> +    }
> +
> +    if (!add_signature_to_cose(cose)) {
> +        goto err;
> +    }
> +
> +    len = cbor_serialize(cose, buf, buf_len);
> +    if (len == 0) {
> +        goto err;
> +    }
> +
> +    root = cbor_new_definite_map(1);
> +    if (!root) {
> +        goto err;
> +    }
> +    if (!qemu_cbor_add_map_to_map(root, "Attestation", 1, &nested_map)) {
> +        goto err;
> +    }
> +    if (!qemu_cbor_add_bytestring_to_map(nested_map, "document", buf, len)) {
> +        goto err;
> +    }
> +
> +    len = cbor_serialize(root, response->iov_base, response->iov_len);
> +    if (len == 0) {
> +        if (error_response(response, NSM_BUFFER_TOO_SMALL, errp)) {
> +            r = true;
> +        }
> +        goto out;
> +    }
> +
> +    response->iov_len = len;
> +    r = true;
> +
> + out:
> +    if (root) {
> +        cbor_decref(&root);
> +    }
> +    if (cose) {
> +        cbor_decref(&cose);
> +    }
> +    g_free(buf);
> +    return r;
> +
> + err:
> +    error_setg(errp, "Failed to initialize Attestation response");
> +    goto out;
> +}
> +

> +static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
> +{
> +    VirtQueueElement *out_elem = NULL;
> +    VirtQueueElement *in_elem = NULL;

Both can be 'g_autofree' , to remove the duplicated g_free calls

> +    VirtIONSM *vnsm = VIRTIO_NSM(vdev);
> +    Error *err = NULL;
> +
> +    out_elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> +    if (!out_elem) {
> +        /* nothing in virtqueue */
> +        return;
> +    }
> +
> +    if (out_elem->out_num != 1) {
> +        virtio_error(vdev, "Expected one request buffer first in virtqueue");
> +        goto cleanup;
> +    }
> +
> +    in_elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> +    if (!in_elem) {
> +        virtio_error(vdev, "Expected response buffer after request buffer "
> +                     "in virtqueue");
> +        goto cleanup;
> +    }
> +    if (in_elem->in_num != 1) {
> +        virtio_error(vdev, "Expected one response buffer after request buffer "
> +                     "in virtqueue");
> +        goto cleanup;
> +    }
> +
> +    if (!get_nsm_request_response(vnsm, out_elem->out_sg, in_elem->in_sg,
> +                                  &err)) {
> +        error_report_err(err);
> +        virtio_error(vdev, "Failed to get NSM request response");
> +        goto cleanup;
> +    }
> +
> +    virtqueue_push(vq, out_elem, 0);
> +    virtqueue_push(vq, in_elem, in_elem->in_sg->iov_len);
> +    g_free(out_elem);
> +    g_free(in_elem);
> +    virtio_notify(vdev, vq);
> +    return;
> +
> + cleanup:
> +    if (out_elem) {
> +        virtqueue_detach_element(vq, out_elem, 0);
> +    }
> +    if (in_elem) {
> +        virtqueue_detach_element(vq, in_elem, 0);
> +    }
> +    g_free(out_elem);
> +    g_free(in_elem);
> +    return;
> +}



> diff --git a/include/hw/virtio/virtio-nsm.h b/include/hw/virtio/virtio-nsm.h
> new file mode 100644
> index 0000000000..7901c19fe4
> --- /dev/null
> +++ b/include/hw/virtio/virtio-nsm.h
> @@ -0,0 +1,59 @@
> +/*
> + * 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.
> + */
> +
> +#ifndef QEMU_VIRTIO_NSM_H
> +#define QEMU_VIRTIO_NSM_H
> +
> +#include "hw/virtio/virtio.h"
> +#include "qom/object.h"
> +
> +#define NSM_MAX_PCRS 32
> +#define NSM_USER_DATA_MAX_SIZE 512
> +#define NSM_NONCE_MAX_SIZE 512
> +#define NSM_PUBLIC_KEY_MAX_SIZE 1024


> +#define SHA384_BYTE_LEN 48

Hmm, you've added this in two different headers now, which suggests
we should have it in a common header. include/qcrypto/hash.h is the
obvious place to put this, so how about adding

  #define QCRYPTO_HASH_DIGEST_LEN_MD5       16
  #define QCRYPTO_HASH_DIGEST_LEN_SHA1      20
  #define QCRYPTO_HASH_DIGEST_LEN_SHA224    28
  #define QCRYPTO_HASH_DIGEST_LEN_SHA256    32
  #define QCRYPTO_HASH_DIGEST_LEN_SHA384    48
  #define QCRYPTO_HASH_DIGEST_LEN_SHA512    64
  #define QCRYPTO_HASH_DIGEST_LEN_RIPEMD160 20

then updating qcrypto_hash_alg_size table to use these constants
too.

THis should be a standalone commit at the start of the series.



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