Re: [PATCH v3 14/15] i386/sev: Add implementation of CGS set_guest_policy()

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

On Fri, Jun 21, 2024 at 03:29:17PM +0100, Roy Hopkins wrote:
> The new cgs_set_guest_policy() function is provided to receive the guest
> policy flags, SNP ID block and SNP ID authentication from guest
> configuration such as an IGVM file and apply it to the platform prior to
> launching the guest.
> 
> The policy is used to populate values for the existing 'policy',
> 'id_block' and 'id_auth' parameters. When provided, the guest policy is
> applied and the ID block configuration is used to verify the launch
> measurement and signatures. The guest is only successfully started if
> the expected launch measurements match the actual measurements and the
> signatures are valid.
> 
> Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> ---
>  target/i386/sev.h | 12 +++++++
>  target/i386/sev.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 95 insertions(+)
> 
> diff --git a/target/i386/sev.h b/target/i386/sev.h
> index 2ccd6fe1e8..7b92102bd0 100644
> --- a/target/i386/sev.h
> +++ b/target/i386/sev.h
> @@ -157,6 +157,18 @@ struct QEMU_PACKED sev_es_save_area {
>      uint8_t fpreg_ymm[256];
>  };
>  
> +struct QEMU_PACKED sev_snp_id_authentication {
> +    uint32_t id_key_alg;
> +    uint32_t auth_key_algo;
> +    uint8_t reserved[56];
> +    uint8_t id_block_sig[512];
> +    uint8_t id_key[1028];
> +    uint8_t reserved2[60];
> +    uint8_t id_key_sig[512];
> +    uint8_t author_key[1028];
> +    uint8_t reserved3[892];
> +};
> +
>  #ifdef CONFIG_SEV
>  bool sev_enabled(void);
>  bool sev_es_enabled(void);
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 5d9ef745bb..688b378c42 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -2446,6 +2446,88 @@ static int cgs_get_mem_map_entry(int index,
>      return 0;
>  }
>  
> +static int cgs_set_guest_policy(ConfidentialGuestPolicyType policy_type,
> +                                uint64_t policy, void *policy_data1,
> +                                uint32_t policy_data1_size, void *policy_data2,
> +                                uint32_t policy_data2_size, Error **errp)
> +{
> +    if (policy_type != GUEST_POLICY_SEV) {
> +        error_setg(errp, "%s: Invalid guest policy type provided for SEV: %d",
> +        __func__, policy_type);
> +        return -1;
> +    }
> +    /*
> +     * SEV-SNP handles policy differently. The policy flags are defined in
> +     * kvm_start_conf.policy and an ID block and ID auth can be provided.
> +     */
> +    if (sev_snp_enabled()) {
> +        SevSnpGuestState *sev_snp_guest =
> +            SEV_SNP_GUEST(MACHINE(qdev_get_machine())->cgs);
> +        struct kvm_sev_snp_launch_finish *finish =
> +            &sev_snp_guest->kvm_finish_conf;
> +
> +        /*
> +         * The policy consists of flags in 'policy' and optionally an ID block
> +         * and ID auth in policy_data1 and policy_data2 respectively. The ID
> +         * block and auth are optional so clear any previous ID block and auth
> +         * and set them if provided, but always set the policy flags.
> +         */
> +        g_free(sev_snp_guest->id_block);
> +        g_free((guchar *)finish->id_block_uaddr);
> +        g_free(sev_snp_guest->id_auth);
> +        g_free((guchar *)finish->id_auth_uaddr);
> +        sev_snp_guest->id_block = NULL;
> +        finish->id_block_uaddr = 0;
> +        sev_snp_guest->id_auth = NULL;
> +        finish->id_auth_uaddr = 0;
> +
> +        if (policy_data1_size > 0) {
> +            struct sev_snp_id_authentication *id_auth =
> +                (struct sev_snp_id_authentication *)policy_data2;
> +
> +            if (policy_data1_size != KVM_SEV_SNP_ID_BLOCK_SIZE) {
> +                error_setg(errp, "%s: Invalid SEV-SNP ID block: incorrect size",
> +                           __func__);
> +                return -1;
> +            }
> +            if (policy_data2_size != KVM_SEV_SNP_ID_AUTH_SIZE) {
> +                error_setg(errp,
> +                           "%s: Invalid SEV-SNP ID auth block: incorrect size",
> +                           __func__);
> +                return -1;
> +            }

Perhaps add

  assert(policy_data1 != NULL);
  assert(policy_data2 != NULL);


> +            finish->id_block_uaddr =
> +                (__u64)g_malloc0(KVM_SEV_SNP_ID_BLOCK_SIZE);
> +            finish->id_authu_addr = (__u64)g_malloc0(KVM_SEV_SNP_ID_AUTH_SIZE);
> +            memcpy((void *)finish->id_block_uaddr, policy_data1,
> +                   KVM_SEV_SNP_ID_BLOCK_SIZE);
> +            memcpy((void *)finish->id_auth_uaddr, policy_data2,
> +                   KVM_SEV_SNP_ID_AUTH_SIZE);

How about using g_memdup2 ?

   finish->id_block_uaddr = (__u64)g_memdup2(policy_data1, KVM_SEV_SNP_ID_BLOCK_SIZE);
   finish->id_auth_uaddr = (__u64)g_memdup2(policy_data2, KVM_SEV_SNP_ID_AUTH_SIZE);

> +
> +            /*
> +             * Check if an author key has been provided and use that to flag
> +             * whether the author key is enabled. The first of the author key
> +             * must be non-zero to indicate the key type, which will currently
> +             * always be 2.
> +             */
> +            sev_snp_guest->kvm_finish_conf.auth_key_en =
> +                id_auth->author_key[0] ? 1 : 0;
> +            finish->id_block_en = 1;
> +        }
> +        sev_snp_guest->kvm_start_conf.policy = policy;
> +    } else {
> +        SevGuestState *sev_guest = SEV_GUEST(MACHINE(qdev_get_machine())->cgs);
> +        /* Only the policy flags are supported for SEV and SEV-ES */
> +        if ((policy_data1_size > 0) || (policy_data2_size > 0) || !sev_guest) {
> +            error_setg(errp, "%s: An ID block/ID auth block has been provided "
> +                             "but SEV-SNP is not supported", __func__);

Slightly more accurate to say  s/is not supported/is not enabled/

> +            return -1;
> +        }
> +        sev_guest->policy = policy;
> +    }
> +    return 0;
> +}
> +
>  static void
>  sev_common_class_init(ObjectClass *oc, void *data)
>  {
> @@ -2484,6 +2566,7 @@ sev_common_instance_init(Object *obj)
>      cgs->check_support = cgs_check_support;
>      cgs->set_guest_state = cgs_set_guest_state;
>      cgs->get_mem_map_entry = cgs_get_mem_map_entry;
> +    cgs->set_guest_policy = cgs_set_guest_policy;
>  
>      QTAILQ_INIT(&sev_common->launch_vmsa);
>  }
> -- 
> 2.43.0
> 

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