Re: [PATCH v3 10/26] target/arm/kvm-rme: Add Realm Personalization Value parameter

[Markus Armbruster]

Jean-Philippe Brucker <jean-philippe@linaro.org> writes:

> The Realm Personalization Value (RPV) is provided by the user to
> distinguish Realms that have the same initial measurement.
>
> The user provides up to 64 hexadecimal bytes. They are stored into the
> RPV in the same order, zero-padded on the right.
>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Cc: Eduardo Habkost <eduardo@habkost.net>
> Acked-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> v2->v3: Fix documentation
> ---
>  qapi/qom.json        |  15 ++++++
>  target/arm/kvm-rme.c | 111 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 126 insertions(+)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index a8beeabf1f..f982850bca 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -1068,6 +1068,19 @@
>    'data': { '*cpu-affinity': ['uint16'],
>              '*node-affinity': ['uint16'] } }
>  
> +##
> +# @RmeGuestProperties:
> +#
> +# Properties for rme-guest objects.
> +#
> +# @personalization-value: Realm personalization value, as a 64-byte
> +#     hex string. This optional parameter allows to uniquely identify
> +#     the VM instance during attestation. (default: 0)

QMP commonly uses base64 for encoding binary data.  Any particular
reason to deviate?

Is the "hex string" to be mapped to binary in little or big endian?  Not
an issue with base64.

Nitpick: (default: all-zero), please, for consistency with similar
documentation in SevSnpGuestProperties.

> +#
> +# Since: 9.3
> +##
> +{ 'struct': 'RmeGuestProperties',
> +  'data': { '*personalization-value': 'str' } }
>  
>  ##
>  # @ObjectType:
> @@ -1121,6 +1134,7 @@
>      { 'name': 'pr-manager-helper',
>        'if': 'CONFIG_LINUX' },
>      'qtest',
> +    'rme-guest',
>      'rng-builtin',
>      'rng-egd',
>      { 'name': 'rng-random',

The commit message claims the patch adds a parameter.  It actually adds
an entire object type.

> @@ -1195,6 +1209,7 @@
>        'pr-manager-helper':          { 'type': 'PrManagerHelperProperties',
>                                        'if': 'CONFIG_LINUX' },
>        'qtest':                      'QtestProperties',
> +      'rme-guest':                  'RmeGuestProperties',
>        'rng-builtin':                'RngProperties',
>        'rng-egd':                    'RngEgdProperties',
>        'rng-random':                 { 'type': 'RngRandomProperties',
> diff --git a/target/arm/kvm-rme.c b/target/arm/kvm-rme.c
> index 83a29421df..0be55867ee 100644
> --- a/target/arm/kvm-rme.c
> +++ b/target/arm/kvm-rme.c
> @@ -23,11 +23,15 @@ OBJECT_DECLARE_SIMPLE_TYPE(RmeGuest, RME_GUEST)
>  
>  #define RME_PAGE_SIZE qemu_real_host_page_size()
>  
> +#define RME_MAX_CFG         1
> +
>  struct RmeGuest {
>      ConfidentialGuestSupport parent_obj;
>      Notifier rom_load_notifier;
>      GSList *ram_regions;
>  
> +    uint8_t *personalization_value;
> +
>      hwaddr ram_base;
>      size_t ram_size;
>  };
> @@ -43,6 +47,48 @@ typedef struct {
>  
>  static RmeGuest *rme_guest;
>  
> +static int rme_configure_one(RmeGuest *guest, uint32_t cfg, Error **errp)
> +{
> +    int ret;
> +    const char *cfg_str;
> +    struct kvm_cap_arm_rme_config_item args = {
> +        .cfg = cfg,
> +    };
> +
> +    switch (cfg) {
> +    case KVM_CAP_ARM_RME_CFG_RPV:
> +        if (!guest->personalization_value) {
> +            return 0;
> +        }
> +        memcpy(args.rpv, guest->personalization_value, 
> KVM_CAP_ARM_RME_RPV_SIZE);
> +        cfg_str = "personalization value";
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }

This function gets called with @cfg arguments 0 .. RME_MAX_CFG-1, from
rme_configure() right below.  RME_MAX_CFG is defined to 1 above.

The switch assumes KVM_CAP_ARM_RME_CFG_RPV is zero.  Such assumptions
are ideally avoided, and alternatively checked at build time.

> +
> +    ret = kvm_vm_enable_cap(kvm_state, KVM_CAP_ARM_RME, 0,
> +                            KVM_CAP_ARM_RME_CONFIG_REALM, (intptr_t)&args);
> +    if (ret) {
> +        error_setg_errno(errp, -ret, "failed to configure %s", cfg_str);
> +    }
> +    return ret;
> +}
> +
> +static int rme_configure(Error **errp)
> +{
> +    int ret;
> +    int cfg;
> +
> +    for (cfg = 0; cfg < RME_MAX_CFG; cfg++) {
> +        ret = rme_configure_one(rme_guest, cfg, errp);
> +        if (ret) {
> +            return ret;
> +        }
> +    }
> +    return 0;
> +}
> +
>  static int rme_init_ram(hwaddr base, size_t size, Error **errp)
>  {
>      int ret;
> @@ -123,6 +169,10 @@ static int rme_create_realm(Error **errp)
>  {
>      int ret;
>  
> +    if (rme_configure(errp)) {
> +        return -1;
> +    }
> +
>      ret = kvm_vm_enable_cap(kvm_state, KVM_CAP_ARM_RME, 0,
>                              KVM_CAP_ARM_RME_CREATE_RD);
>      if (ret) {
> @@ -168,8 +218,69 @@ static void rme_vm_state_change(void *opaque, bool 
> running, RunState state)
>      }
>  }
>  
> +static char *rme_get_rpv(Object *obj, Error **errp)
> +{
> +    RmeGuest *guest = RME_GUEST(obj);
> +    GString *s;
> +    int i;
> +
> +    if (!guest->personalization_value) {
> +        return NULL;
> +    }
> +
> +    s = g_string_sized_new(KVM_CAP_ARM_RME_RPV_SIZE * 2 + 1);
> +
> +    for (i = 0; i < KVM_CAP_ARM_RME_RPV_SIZE; i++) {
> +        g_string_append_printf(s, "%02x", guest->personalization_value[i]);
> +    }

The size of the destrination string is known at compile time.  Why grow
it dynamically?

Base64 would take less code.

> +
> +    return g_string_free(s, /* free_segment */ false);
> +}
> +
> +static void rme_set_rpv(Object *obj, const char *value, Error **errp)
> +{
> +    RmeGuest *guest = RME_GUEST(obj);
> +    size_t len = strlen(value);
> +    uint8_t *out;
> +    int i = 1;
> +    int ret;
> +
> +    g_free(guest->personalization_value);
> +    guest->personalization_value = out = g_malloc0(KVM_CAP_ARM_RME_RPV_SIZE);
> +
> +    /* Two chars per byte */
> +    if (len > KVM_CAP_ARM_RME_RPV_SIZE * 2) {
> +        error_setg(errp, "Realm Personalization Value is too large");
> +        return;
> +    }
> +
> +    /* First byte may have a single char */
> +    if (len % 2) {
> +        ret = sscanf(value, "%1hhx", out++);
> +    } else {
> +        ret = sscanf(value, "%2hhx", out++);
> +        i++;
> +    }
> +    if (ret != 1) {
> +        error_setg(errp, "Invalid Realm Personalization Value");
> +        return;
> +    }
> +
> +    for (; i < len; i += 2) {
> +        ret = sscanf(value + i, "%2hhx", out++);
> +        if (ret != 1) {
> +            error_setg(errp, "Invalid Realm Personalization Value");
> +            return;
> +        }
> +    }

Looks like this supports hex strings shorter than
KVM_CAP_ARM_RME_RPV_SIZE * 2.  How these get padded is not documented.
Fixable, but are you sure the convenience is worth the complexity?

Again, base64 would take less code.

> +}
> +
>  static void rme_guest_class_init(ObjectClass *oc, void *data)
>  {
> +    object_class_property_add_str(oc, "personalization-value", rme_get_rpv,
> +                                  rme_set_rpv);
> +    object_class_property_set_description(oc, "personalization-value",
> +            "Realm personalization value (512-bit hexadecimal number)");
>  }
>  
>  static void rme_guest_init(Object *obj)