Re: [PATCH v4 03/14] migration.json: add AMD SEV specific migration parameters

[Ashish Kalra]

Hello Dov,

On Thu, Aug 05, 2021 at 12:42:50PM +0300, Dov Murik wrote:
> 
> 
> On 04/08/2021 14:54, Ashish Kalra wrote:
> > From: Brijesh Singh <brijesh.singh@amd.com>
> > 
> > AMD SEV migration flow requires that target machine's public Diffie-Hellman
> > key (PDH) and certificate chain must be passed before initiating the guest
> > migration. User can use QMP 'migrate-set-parameters' to pass the certificate
> > chain. The certificate chain will be used while creating the outgoing
> > encryption context.
> 
> Just making sure: The source QEMU must *not* accept the sev_amd_cert
> from the target, because that will allow a malicious target to give its
> own root cert instead of the official AMD one.  Instead it should use
> its own trusted AMD root certificate.
> 

Actually, it is the responsibility of the VMM management stack to manage
all the certificates, before starting migration the VMM management stack
will retrieve the certs, from either the target or other ways.

KVM/qemu are just facilitating in providing these certs to the PSP via
the SEND_START command, it is the responsibility of the PSP to
validate/authenticate these certs and establish an encryption context
with the target.

Thanks,
Ashish

> 
> > 
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> > ---
> >  migration/migration.c | 61 +++++++++++++++++++++++++++++++++++++++++++
> >  monitor/hmp-cmds.c    | 18 +++++++++++++
> >  qapi/migration.json   | 40 +++++++++++++++++++++++++---
> >  3 files changed, 116 insertions(+), 3 deletions(-)
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 041b8451a6..daea3ecd04 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -907,6 +907,12 @@ MigrationParameters 
> > *qmp_query_migrate_parameters(Error **errp)
> >      params->announce_rounds = s->parameters.announce_rounds;
> >      params->has_announce_step = true;
> >      params->announce_step = s->parameters.announce_step;
> > +    params->has_sev_pdh = true;
> > +    params->sev_pdh = g_strdup(s->parameters.sev_pdh);
> > +    params->has_sev_plat_cert = true;
> > +    params->sev_plat_cert = g_strdup(s->parameters.sev_plat_cert);
> > +    params->has_sev_amd_cert = true;
> > +    params->sev_amd_cert = g_strdup(s->parameters.sev_amd_cert);
> > 
> >      if (s->parameters.has_block_bitmap_mapping) {
> >          params->has_block_bitmap_mapping = true;
> > @@ -1563,6 +1569,18 @@ static void 
> > migrate_params_test_apply(MigrateSetParameters *params,
> >          dest->has_block_bitmap_mapping = true;
> >          dest->block_bitmap_mapping = params->block_bitmap_mapping;
> >      }
> > +    if (params->has_sev_pdh) {
> > +        assert(params->sev_pdh->type == QTYPE_QSTRING);
> > +        dest->sev_pdh = g_strdup(params->sev_pdh->u.s);
> > +    }
> > +    if (params->has_sev_plat_cert) {
> > +        assert(params->sev_plat_cert->type == QTYPE_QSTRING);
> > +        dest->sev_plat_cert = g_strdup(params->sev_plat_cert->u.s);
> > +    }
> > +    if (params->has_sev_amd_cert) {
> > +        assert(params->sev_amd_cert->type == QTYPE_QSTRING);
> > +        dest->sev_amd_cert = g_strdup(params->sev_amd_cert->u.s);
> > +    }
> >  }
> > 
> >  static void migrate_params_apply(MigrateSetParameters *params, Error 
> > **errp)
> > @@ -1685,6 +1703,21 @@ static void 
> > migrate_params_apply(MigrateSetParameters *params, Error **errp)
> >              QAPI_CLONE(BitmapMigrationNodeAliasList,
> >                         params->block_bitmap_mapping);
> >      }
> > +    if (params->has_sev_pdh) {
> > +        g_free(s->parameters.sev_pdh);
> > +        assert(params->sev_pdh->type == QTYPE_QSTRING);
> > +        s->parameters.sev_pdh = g_strdup(params->sev_pdh->u.s);
> > +    }
> > +    if (params->has_sev_plat_cert) {
> > +        g_free(s->parameters.sev_plat_cert);
> > +        assert(params->sev_plat_cert->type == QTYPE_QSTRING);
> > +        s->parameters.sev_plat_cert = g_strdup(params->sev_plat_cert->u.s);
> > +    }
> > +    if (params->has_sev_amd_cert) {
> > +        g_free(s->parameters.sev_amd_cert);
> > +        assert(params->sev_amd_cert->type == QTYPE_QSTRING);
> > +        s->parameters.sev_amd_cert = g_strdup(params->sev_amd_cert->u.s);
> > +    }
> >  }
> > 
> >  void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
> > @@ -1705,6 +1738,27 @@ void qmp_migrate_set_parameters(MigrateSetParameters 
> > *params, Error **errp)
> >          params->tls_hostname->type = QTYPE_QSTRING;
> >          params->tls_hostname->u.s = strdup("");
> >      }
> > +    /* TODO Rewrite "" to null instead */
> > +    if (params->has_sev_pdh
> > +        && params->sev_pdh->type == QTYPE_QNULL) {
> > +        qobject_unref(params->sev_pdh->u.n);
> > +        params->sev_pdh->type = QTYPE_QSTRING;
> > +        params->sev_pdh->u.s = strdup("");
> > +    }
> > +    /* TODO Rewrite "" to null instead */
> > +    if (params->has_sev_plat_cert
> > +        && params->sev_plat_cert->type == QTYPE_QNULL) {
> > +        qobject_unref(params->sev_plat_cert->u.n);
> > +        params->sev_plat_cert->type = QTYPE_QSTRING;
> > +        params->sev_plat_cert->u.s = strdup("");
> > +    }
> > +    /* TODO Rewrite "" to null instead */
> > +    if (params->has_sev_amd_cert
> > +        && params->sev_amd_cert->type == QTYPE_QNULL) {
> > +        qobject_unref(params->sev_amd_cert->u.n);
> > +        params->sev_amd_cert->type = QTYPE_QSTRING;
> > +        params->sev_amd_cert->u.s = strdup("");
> > +    }
> > 
> >      migrate_params_test_apply(params, &tmp);
> > 
> > @@ -4233,6 +4287,9 @@ static void migration_instance_finalize(Object *obj)
> >      qemu_mutex_destroy(&ms->qemu_file_lock);
> >      g_free(params->tls_hostname);
> >      g_free(params->tls_creds);
> > +    g_free(params->sev_pdh);
> > +    g_free(params->sev_plat_cert);
> > +    g_free(params->sev_amd_cert);
> >      qemu_sem_destroy(&ms->wait_unplug_sem);
> >      qemu_sem_destroy(&ms->rate_limit_sem);
> >      qemu_sem_destroy(&ms->pause_sem);
> > @@ -4280,6 +4337,10 @@ static void migration_instance_init(Object *obj)
> >      params->has_announce_rounds = true;
> >      params->has_announce_step = true;
> > 
> > +    params->sev_pdh = g_strdup("");
> > +    params->sev_plat_cert = g_strdup("");
> > +    params->sev_amd_cert = g_strdup("");
> > +
> 
> TODO: init to NULL instead.
> 
> >      qemu_sem_init(&ms->postcopy_pause_sem, 0);
> >      qemu_sem_init(&ms->postcopy_pause_rp_sem, 0);
> >      qemu_sem_init(&ms->rp_state.rp_sem, 0);
> > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> > index e00255f7ee..27ca2024bb 100644
> > --- a/monitor/hmp-cmds.c
> > +++ b/monitor/hmp-cmds.c
> > @@ -1399,6 +1399,24 @@ void hmp_migrate_set_parameter(Monitor *mon, const 
> > QDict *qdict)
> >          error_setg(&err, "The block-bitmap-mapping parameter can only be 
> > set "
> >                     "through QMP");
> >          break;
> > +    case MIGRATION_PARAMETER_SEV_PDH:
> > +        p->has_sev_pdh = true;
> > +        p->sev_pdh = g_new0(StrOrNull, 1);
> > +        p->sev_pdh->type = QTYPE_QSTRING;
> > +        visit_type_str(v, param, &p->sev_pdh->u.s, &err);
> > +        break;
> > +    case MIGRATION_PARAMETER_SEV_PLAT_CERT:
> > +        p->has_sev_plat_cert = true;
> > +        p->sev_plat_cert = g_new0(StrOrNull, 1);
> > +        p->sev_plat_cert->type = QTYPE_QSTRING;
> > +        visit_type_str(v, param, &p->sev_plat_cert->u.s, &err);
> > +        break;
> > +    case MIGRATION_PARAMETER_SEV_AMD_CERT:
> > +        p->has_sev_amd_cert = true;
> > +        p->sev_amd_cert = g_new0(StrOrNull, 1);
> > +        p->sev_amd_cert->type = QTYPE_QSTRING;
> > +        visit_type_str(v, param, &p->sev_amd_cert->u.s, &err);
> > +        break;
> >      default:
> >          assert(0);
> >      }
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index 1124a2dda8..69c615ec4d 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -743,6 +743,15 @@
> >  #                        block device name if there is one, and to their 
> > node name
> >  #                        otherwise. (Since 5.2)
> >  #
> > +# @sev-pdh: The target host platform diffie-hellman key encoded in base64
> > +#           (Since 4.2)
> 
> Since 6.2, I assume. (same for all the changes in this file)
> 
> 
> > +#
> > +# @sev-plat-cert: The target host platform certificate chain encoded in 
> > base64
> > +#                 (Since 4.2)
> > +#
> > +# @sev-amd-cert: AMD certificate chain which include ASK and OCA encoded in
> > +#                base64 (Since 4.2)
> > +#
> >  # Since: 2.4
> >  ##
> >  { 'enum': 'MigrationParameter',
> > @@ -758,7 +767,8 @@
> >             'xbzrle-cache-size', 'max-postcopy-bandwidth',
> >             'max-cpu-throttle', 'multifd-compression',
> >             'multifd-zlib-level' ,'multifd-zstd-level',
> > -           'block-bitmap-mapping' ] }
> > +           'block-bitmap-mapping',
> > +           'sev-pdh', 'sev-plat-cert', 'sev-amd-cert' ] }
> > 
> >  ##
> >  # @MigrateSetParameters:
> > @@ -903,6 +913,15 @@
> >  #                        block device name if there is one, and to their 
> > node name
> >  #                        otherwise. (Since 5.2)
> >  #
> > +# @sev-pdh: The target host platform diffie-hellman key encoded in base64
> > +#           (Since 4.2)
> > +#
> > +# @sev-plat-cert: The target host platform certificate chain encoded in 
> > base64
> > +#                 (Since 4.2)
> > +#
> > +# @sev-amd-cert: AMD certificate chain which include ASK and OCA encoded in
> > +#                base64 (Since 4.2)
> > +#
> >  # Since: 2.4
> >  ##
> >  # TODO either fuse back into MigrationParameters, or make
> > @@ -934,7 +953,10 @@
> >              '*multifd-compression': 'MultiFDCompression',
> >              '*multifd-zlib-level': 'uint8',
> >              '*multifd-zstd-level': 'uint8',
> > -            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
> > +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
> > +            '*sev-pdh':'StrOrNull',
> > +            '*sev-plat-cert': 'StrOrNull',
> > +            '*sev-amd-cert' : 'StrOrNull' } }
> > 
> >  ##
> >  # @migrate-set-parameters:
> > @@ -1099,6 +1121,15 @@
> >  #                        block device name if there is one, and to their 
> > node name
> >  #                        otherwise. (Since 5.2)
> >  #
> > +# @sev-pdh: The target host platform diffie-hellman key encoded in base64
> > +#           (Since 4.2)
> > +#
> > +# @sev-plat-cert: The target host platform certificate chain encoded in 
> > base64
> > +#                 (Since 4.2)
> > +#
> > +# @sev-amd-cert: AMD certificate chain which include ASK and OCA encoded in
> > +#                base64 (Since 4.2)
> > +#
> >  # Since: 2.4
> >  ##
> >  { 'struct': 'MigrationParameters',
> > @@ -1128,7 +1159,10 @@
> >              '*multifd-compression': 'MultiFDCompression',
> >              '*multifd-zlib-level': 'uint8',
> >              '*multifd-zstd-level': 'uint8',
> > -            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
> > +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
> > +            '*sev-pdh':'str',
> > +            '*sev-plat-cert': 'str',
> > +            '*sev-amd-cert' : 'str'} }
> > 
> >  ##
> >  # @query-migrate-parameters:
> >