On Sat, Sep 04, 2021 at 12:12:19AM +0300, Dov Murik wrote:
>
>
> On 27/08/2021 1:26, Michael Roth wrote:
> > From: Brijesh Singh <brijesh.singh@xxxxxxx>
> >
> > SEV-SNP support relies on a different set of properties/state than the
> > existing 'sev-guest' object. This patch introduces the 'sev-snp-guest'
> > object, which can be used to configure an SEV-SNP guest. For example,
> > a default-configured SEV-SNP guest with no additional information
> > passed in for use with attestation:
> >
> > -object sev-snp-guest,id=sev0
> >
> > or a fully-specified SEV-SNP guest where all spec-defined binary
> > blobs are passed in as base64-encoded strings:
> >
> > -object sev-snp-guest,id=sev0, \
> > policy=0x30000, \
> > init-flags=0, \
> > id-block=YWFhYWFhYWFhYWFhYWFhCg==, \
> > id-auth=CxHK/OKLkXGn/KpAC7Wl1FSiisWDbGTEKz..., \
> > auth-key-enabled=on, \
> > host-data=LNkCWBRC5CcdGXirbNUV1OrsR28s..., \
> > guest-visible-workarounds=AA==, \
> >
> > See the QAPI schema updates included in this patch for more usage
> > details.
> >
> > In some cases these blobs may be up to 4096 characters, but this is
> > generally well below the default limit for linux hosts where
> > command-line sizes are defined by the sysconf-configurable ARG_MAX
> > value, which defaults to 2097152 characters for Ubuntu hosts, for
> > example.
> >
> > Co-developed-by: Michael Roth <michael.roth@xxxxxxx>
> > Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
> > Signed-off-by: Michael Roth <michael.roth@xxxxxxx>
> > ---
> > docs/amd-memory-encryption.txt | 77 ++++++++++-
> > qapi/qom.json | 60 ++++++++
> > target/i386/sev.c | 245 ++++++++++++++++++++++++++++++++-
> > 3 files changed, 379 insertions(+), 3 deletions(-)
> >
> > diff --git a/docs/amd-memory-encryption.txt b/docs/amd-memory-encryption.txt
> > index ffca382b5f..0d82e67fa1 100644
> > --- a/docs/amd-memory-encryption.txt
> > +++ b/docs/amd-memory-encryption.txt
> > @@ -22,8 +22,8 @@ support for notifying a guest's operating system when certain types of VMEXITs
> > are about to occur. This allows the guest to selectively share information with
> > the hypervisor to satisfy the requested function.
> >
> > -Launching
> > ----------
> > +Launching (SEV and SEV-ES)
> > +--------------------------
> > Boot images (such as bios) must be encrypted before a guest can be booted. The
> > MEMORY_ENCRYPT_OP ioctl provides commands to encrypt the images: LAUNCH_START,
> > LAUNCH_UPDATE_DATA, LAUNCH_MEASURE and LAUNCH_FINISH. These four commands
> > @@ -113,6 +113,79 @@ a SEV-ES guest:
> > - Requires in-kernel irqchip - the burden is placed on the hypervisor to
> > manage booting APs.
> >
> > +Launching (SEV-SNP)
> > +-------------------
> > +Boot images (such as bios) must be encrypted before a guest can be booted. The
> > +MEMORY_ENCRYPT_OP ioctl provides commands to encrypt the images:
> > +KVM_SNP_INIT, SNP_LAUNCH_START, SNP_LAUNCH_UPDATE, and SNP_LAUNCH_FINISH. These
> > +four commands together generate a fresh memory encryption key for the VM,
> > +encrypt the boot images for a successful launch.
> > +
> > +KVM_SNP_INIT is called first to initialize the SEV-SNP firmware and SNP
> > +features in the KVM. The feature flags value can be provided through the
> > +'init-flags' property of the 'sev-snp-guest' object.
> > +
> > ++------------+-------+----------+---------------------------------+
> > +| key | type | default | meaning |
> > ++------------+-------+----------+---------------------------------+
> > +| init_flags | hex | 0 | SNP feature flags |
> > ++-----------------------------------------------------------------+
> > +
> > +Note: currently the init_flags must be zero.
> > +
> > +SNP_LAUNCH_START is called first to create a cryptographic launch context
> > +within the firmware. To create this context, guest owner must provide a guest
> > +policy and other parameters as described in the SEV-SNP firmware
> > +specification. The launch parameters should be specified as described in the
> > +QAPI schema for the 'sev-snp-guest' object.
> > +
> > +The SNP_LAUNCH_START uses the following parameters (see the SEV-SNP
> > +specification for more details):
> > +
> > ++--------+-------+----------+----------------------------------------------+
> > +| key | type | default | meaning |
> > ++--------+-------+----------+----------------------------------------------+
> > +| policy | hex | 0x30000 | a 64-bit guest policy |
> > +| imi_en | bool | 0 | 1 when IMI is enabled |
> > +| ma_end | bool | 0 | 1 when migration agent is used |
> > +| gosvw | string| 0 | 16-byte base64 encoded string for the guest |
> > +| | | | OS visible workaround. |
> > ++--------+-------+----------+----------------------------------------------+
> > +
> > +SNP_LAUNCH_UPDATE encrypts the memory region using the cryptographic context
> > +created via the SNP_LAUNCH_START command. If required, this command can be called
> > +multiple times to encrypt different memory regions. The command also calculates
> > +the measurement of the memory contents as it encrypts.
> > +
> > +SNP_LAUNCH_FINISH finalizes the guest launch flow. Optionally, while finalizing
> > +the launch the firmware can perform checks on the launch digest computing
> > +through the SNP_LAUNCH_UPDATE. To perform the check the user must supply
> > +the id block, authentication blob and host data that should be included in the
> > +attestation report. See the SEV-SNP spec for further details.
> > +
> > +The SNP_LAUNCH_FINISH uses the following parameters, which can be configured
> > +by the corresponding parameters documented in the QAPI schema for the
> > +'sev-snp-guest' object.
> > +
> > ++------------+-------+----------+----------------------------------------------+
> > +| key | type | default | meaning |
> > ++------------+-------+----------+----------------------------------------------+
> > +| id_block | string| none | base64 encoded ID block |
> > ++------------+-------+----------+----------------------------------------------+
> > +| id_auth | string| none | base64 encoded authentication information |
> > ++------------+-------+----------+----------------------------------------------+
> > +| auth_key_en| bool | 0 | auth block contains author key |
> > ++------------+-------+----------+----------------------------------------------+
> > +| host_data | string| none | host provided data |
> > ++------------+-------+----------+----------------------------------------------+
> > +
> > +To launch a SEV-SNP guest (additional parameters are documented in the QAPI
> > +schema for the 'sev-snp-guest' object):
> > +
> > +# ${QEMU} \
> > + -machine ...,confidential-guest-support=sev0 \
> > + -object sev-snp-guest,id=sev0,cbitpos=51,reduced-phys-bits=1
> > +
> > Debugging
> > -----------
> > Since the memory contents of a SEV guest are encrypted, hypervisor access to
> > diff --git a/qapi/qom.json b/qapi/qom.json
> > index 211e083727..ea39585026 100644
> > --- a/qapi/qom.json
> > +++ b/qapi/qom.json
> > @@ -775,6 +775,64 @@
> > '*policy': 'uint32',
> > '*handle': 'uint32' } }
> >
> > +##
> > +# @SevSnpGuestProperties:
> > +#
> > +# Properties for sev-snp-guest objects. Many of these are direct arguments
> > +# for the SEV-SNP KVM interfaces documented in the linux kernel source
> > +# documentation under 'amd-memory-encryption.rst'. Additional documentation
> > +# is also available in the QEMU source tree under
> > +# 'amd-memory-encryption.rst'.
> > +#
> > +# In addition to those files, please see the SEV-SNP Firmware Specification
> > +# (Rev 0.9) documentation for the SNP_INIT and
> > +# SNP_LAUNCH_{START,UPDATE,FINISH} firmware interfaces, which the KVM
> > +# interfaces are written against.
> > +#
> > +# @init-flags: as documented for the 'flags' parameter of the
> > +# KVM_SNP_INIT KVM command (default: 0)
> > +#
> > +# @policy: as documented for the 'policy' parameter of the
> > +# KVM_SNP_LAUNCH_START KVM command (default: 0x30000)
> > +#
> > +# @guest-visible-workarounds: 16-byte, base64-encoded blob to report
> > +# hypervisor-defined workarounds, as documented
> > +# for the 'gosvm' parameter of the
>
> typo: s/gosvm/gosvw/
Argh, can't seem to get that one right!
>
>
> > +# KVM_SNP_LAUNCH_START KVM command.
> > +# (default: all-zero)
> > +#
> > +# @id-block: 8-byte, base64-encoded blob to provide the ID Block
> > +# structure documented in SEV-SNP spec, as documented for the
> > +# 'id_block_uaddr' parameter of the KVM_SNP_LAUNCH_FINISH
> > +# command (default: all-zero)
>
> The documentation says the ID Block is 96 bytes long (Table 65 in
> section 8.15 of the SNP FW ABI document).
Thanks for the catch, I think I grabbed the value from the wrong column here.
>
>
> > +#
> > +# @id-auth: 4096-byte, base64-encoded blob to provide the ID Authentication
> > +# Information Structure documented in SEV-SNP spec, as documented
> > +# for the 'id_auth_uaddr' parameter of the KVM_SNP_LAUNCH_FINISH
> > +# command (default: all-zero)
> > +#
> > +# @auth-key-enabled: true if 'id-auth' blob contains the Author Key
> > +# documented in the SEV-SNP spec, as documented for the
> > +# 'auth_key_en' parameter of the KVM_SNP_LAUNCH_FINISH
> > +# command (default: false)
> > +#
> > +# @host-data: 32-byte, base64-encoded user-defined blob to provide to the
> > +# guest, as documented for the 'host_data' parameter of the
> > +# KVM_SNP_LAUNCH_FINISH command (default: all-zero)
> > +#
> > +# Since: 6.2
> > +##
> > +{ 'struct': 'SevSnpGuestProperties',
> > + 'base': 'SevCommonProperties',
> > + 'data': {
> > + '*init-flags': 'uint64',
> > + '*policy': 'uint64',
> > + '*guest-visible-workarounds': 'str',
> > + '*id-block': 'str',
> > + '*id-auth': 'str',
> > + '*auth-key-enabled': 'bool',
> > + '*host-data': 'str' } }
> > +
> > ##
> > # @ObjectType:
> > #
> > @@ -816,6 +874,7 @@
> > 'secret',
> > 'secret_keyring',
> > 'sev-guest',
> > + 'sev-snp-guest',
> > 's390-pv-guest',
> > 'throttle-group',
> > 'tls-creds-anon',
> > @@ -873,6 +932,7 @@
> > 'secret': 'SecretProperties',
> > 'secret_keyring': 'SecretKeyringProperties',
> > 'sev-guest': 'SevGuestProperties',
> > + 'sev-snp-guest': 'SevSnpGuestProperties',
> > 'throttle-group': 'ThrottleGroupProperties',
> > 'tls-creds-anon': 'TlsCredsAnonProperties',
> > 'tls-creds-psk': 'TlsCredsPskProperties',
> > diff --git a/target/i386/sev.c b/target/i386/sev.c
> > index 6acebfbd53..ba08b7d3ab 100644
> > --- a/target/i386/sev.c
> > +++ b/target/i386/sev.c
> > @@ -38,7 +38,8 @@
> > OBJECT_DECLARE_SIMPLE_TYPE(SevCommonState, SEV_COMMON)
> > #define TYPE_SEV_GUEST "sev-guest"
> > OBJECT_DECLARE_SIMPLE_TYPE(SevGuestState, SEV_GUEST)
> > -
> > +#define TYPE_SEV_SNP_GUEST "sev-snp-guest"
> > +OBJECT_DECLARE_SIMPLE_TYPE(SevSnpGuestState, SEV_SNP_GUEST)
> >
> > /**
> > * SevGuestState:
> > @@ -82,8 +83,23 @@ struct SevGuestState {
> > char *session_file;
> > };
> >
> > +struct SevSnpGuestState {
> > + SevCommonState sev_common;
> > +
> > + /* configuration parameters */
> > + char *guest_visible_workarounds;
> > + char *id_block;
> > + char *id_auth;
> > + char *host_data;
> > +
> > + struct kvm_snp_init kvm_init_conf;
> > + struct kvm_sev_snp_launch_start kvm_start_conf;
> > + struct kvm_sev_snp_launch_finish kvm_finish_conf;
> > +};
> > +
> > #define DEFAULT_GUEST_POLICY 0x1 /* disable debug */
> > #define DEFAULT_SEV_DEVICE "/dev/sev"
> > +#define DEFAULT_SEV_SNP_POLICY 0x30000
> >
> > #define SEV_INFO_BLOCK_GUID "00f771de-1a7e-4fcb-890e-68c77e2fb44e"
> > typedef struct __attribute__((__packed__)) SevInfoBlock {
> > @@ -364,6 +380,232 @@ static const TypeInfo sev_guest_info = {
> > .class_init = sev_guest_class_init,
> > };
> >
> > +static void
> > +sev_snp_guest_get_init_flags(Object *obj, Visitor *v, const char *name,
> > + void *opaque, Error **errp)
> > +{
> > + visit_type_uint64(v, name,
> > + (uint64_t *)&SEV_SNP_GUEST(obj)->kvm_init_conf.flags,
> > + errp);
> > +}
> > +
> > +static void
> > +sev_snp_guest_set_init_flags(Object *obj, Visitor *v, const char *name,
> > + void *opaque, Error **errp)
> > +{
> > + visit_type_uint64(v, name,
> > + (uint64_t *)&SEV_SNP_GUEST(obj)->kvm_init_conf.flags,
> > + errp);
> > +}
> > +
> > +static void
> > +sev_snp_guest_get_policy(Object *obj, Visitor *v, const char *name,
> > + void *opaque, Error **errp)
> > +{
> > + visit_type_uint64(v, name,
> > + (uint64_t *)&SEV_SNP_GUEST(obj)->kvm_start_conf.policy,
> > + errp);
> > +}
> > +
> > +static void
> > +sev_snp_guest_set_policy(Object *obj, Visitor *v, const char *name,
> > + void *opaque, Error **errp)
> > +{
> > + visit_type_uint64(v, name,
> > + (uint64_t *)&SEV_SNP_GUEST(obj)->kvm_start_conf.policy,
> > + errp);
> > +}
> > +
> > +static char *
> > +sev_snp_guest_get_guest_visible_workarounds(Object *obj, Error **errp)
> > +{
> > + return g_strdup(SEV_SNP_GUEST(obj)->guest_visible_workarounds);
> > +}
> > +
> > +static void
> > +sev_snp_guest_set_guest_visible_workarounds(Object *obj, const char *value,
> > + Error **errp)
> > +{
> > + SevSnpGuestState *sev_snp_guest = SEV_SNP_GUEST(obj);
> > + struct kvm_sev_snp_launch_start *start = &sev_snp_guest->kvm_start_conf;
> > + g_autofree guchar *blob;
> > + gsize len;
> > +
> > + if (sev_snp_guest->guest_visible_workarounds) {
> > + g_free(sev_snp_guest->guest_visible_workarounds);
> > + }
> > +
> > + /* store the base64 str so we don't need to re-encode in getter */
> > + sev_snp_guest->guest_visible_workarounds = g_strdup(value);
> > +
> > + blob = g_base64_decode(sev_snp_guest->guest_visible_workarounds, &len);
>
> I see there's a qbase64_decode which performs some checks and then calls
> g_base64_decode. It might detect illegal chars in the value?
That does seems to be the preferred approach, I'll switch to that for
the series.
>
> Also I think you should verify this decode succeeds by checking that
> blob is not NULL.
>
> (similar comments for all base64_decode calls in this file.)
I'll add those checks as well.
>
>
> > + if (len > sizeof(start->gosvw)) {
> > + error_setg(errp, "parameter length of %lu exceeds max of %lu",
> > + len, sizeof(start->gosvw));
> > + return;
> > + }
> > +
> > + memcpy(start->gosvw, blob, len);
> > +}
> > +
> > +static char *
> > +sev_snp_guest_get_id_block(Object *obj, Error **errp)
> > +{
> > + SevSnpGuestState *sev_snp_guest = SEV_SNP_GUEST(obj);
> > +
> > + return g_strdup(sev_snp_guest->id_block);
> > +}
> > +
> > +static void
> > +sev_snp_guest_set_id_block(Object *obj, const char *value, Error **errp)
> > +{
> > + SevSnpGuestState *sev_snp_guest = SEV_SNP_GUEST(obj);
> > + struct kvm_sev_snp_launch_finish *finish = &sev_snp_guest->kvm_finish_conf;
> > + gsize len;
> > +
> > + if (sev_snp_guest->id_block) {
> > + g_free(sev_snp_guest->id_block);
> > + g_free((guchar *)finish->id_block_uaddr);
> > + }
> > +
> > + /* store the base64 str so we don't need to re-encode in getter */
> > + sev_snp_guest->id_block = g_strdup(value);
> > +
> > + finish->id_block_uaddr = (uint64_t)g_base64_decode(sev_snp_guest->id_block, &len);
> > + if (len > KVM_SEV_SNP_ID_BLOCK_SIZE) {
> > + error_setg(errp, "parameter length of %lu exceeds max of %u",
> > + len, KVM_SEV_SNP_ID_BLOCK_SIZE);
> > + return;
> > + }
> > + finish->id_block_en = 1;
>
> There's no way to set the id_block to a "don't want an ID block", except
> for not giving this option to the sev-snp-guest object. I'm not sure if
> this is a problem (for example, if you dump one VM's config and try to
> load it elsewhere).
>
> Maybe if strlen(value)==0 you should set finish->id_block_en = 0.
That makes sense, I'll add the handling for this.
