Re: [PATCH v3 3/6] conf: refactor launch security to allow more types

On Tue, Jun 22, 2021 at 03:10:46PM +0200, Boris Fiuczynski wrote: > Adding virDomainSecDef for general launch security data > and moving virDomainSEVDef as an element for SEV data. > > Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > --- > src/conf/domain_conf.c | 127 +++++++++++++++++++++++------------- > src/conf/domain_conf.h | 11 +++- > src/conf/virconftypes.h | 2 + > src/qemu/qemu_cgroup.c | 4 +- > src/qemu/qemu_command.c | 44 +++++++++++-- > src/qemu/qemu_driver.c | 3 +- > src/qemu/qemu_firmware.c | 33 ++++++---- > src/qemu/qemu_namespace.c | 20 ++++-- > src/qemu/qemu_process.c | 33 ++++++++-- > src/qemu/qemu_validate.c | 22 +++++-- > src/security/security_dac.c | 6 +- > 11 files changed, 218 insertions(+), 87 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 93ec50ff5d..2bd5210a16 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -3502,6 +3502,19 @@ virDomainSEVDefFree(virDomainSEVDef *def) > g_free(def); > } > > + > +void > +virDomainSecDefFree(virDomainSecDef *def) > +{ > + if (!def) > + return; > + > + virDomainSEVDefFree(def->sev); > + > + g_free(def); > +} > + > + > static void > virDomainOSDefClear(virDomainOSDef *os) > { > @@ -3703,7 +3716,7 @@ void virDomainDefFree(virDomainDef *def) > if (def->namespaceData && def->ns.free) > (def->ns.free)(def->namespaceData); > > - virDomainSEVDefFree(def->sev); > + virDomainSecDefFree(def->sec); > > xmlFreeNode(def->metadata); > > @@ -14720,57 +14733,72 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, > { > VIR_XPATH_NODE_AUTORESTORE(ctxt) > unsigned long policy; > - g_autofree char *type = NULL; > int rc = -1; > > g_autoptr(virDomainSEVDef) def = g_new0(virDomainSEVDef, 1); > > ctxt->node = sevNode; > > - if (!(type = virXMLPropString(sevNode, "type"))) { > + if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) { > virReportError(VIR_ERR_XML_ERROR, "%s", > - _("missing launch security type")); > + _("failed to get launch security policy for " > + "launch security type SEV")); > return NULL; > } > > - def->sectype = virDomainLaunchSecurityTypeFromString(type); > - switch ((virDomainLaunchSecurity) def->sectype) { > - case VIR_DOMAIN_LAUNCH_SECURITY_SEV: > - if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) { > - virReportError(VIR_ERR_XML_ERROR, "%s", > - _("failed to get launch security policy for " > - "launch security type SEV")); > - return NULL; > - } > + /* the following attributes are platform dependent and if missing, > + * we can autofill them from domain capabilities later > + */ > + rc = virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos); > + if (rc == 0) { > + def->haveCbitpos = true; > + } else if (rc == -2) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("Invalid format for launch security cbitpos")); > + return NULL; > + } > > - /* the following attributes are platform dependent and if missing, > - * we can autofill them from domain capabilities later > - */ > - rc = virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos); > - if (rc == 0) { > - def->haveCbitpos = true; > - } else if (rc == -2) { > - virReportError(VIR_ERR_XML_ERROR, "%s", > - _("Invalid format for launch security cbitpos")); > - return NULL; > - } > + rc = virXPathUInt("string(./reducedPhysBits)", ctxt, > + &def->reduced_phys_bits); > + if (rc == 0) { > + def->haveReducedPhysBits = true; > + } else if (rc == -2) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("Invalid format for launch security " > + "reduced-phys-bits")); > + return NULL; > + } > > - rc = virXPathUInt("string(./reducedPhysBits)", ctxt, > - &def->reduced_phys_bits); > - if (rc == 0) { > - def->haveReducedPhysBits = true; > - } else if (rc == -2) { > - virReportError(VIR_ERR_XML_ERROR, "%s", > - _("Invalid format for launch security " > - "reduced-phys-bits")); > - return NULL; > - } > + def->policy = policy; > + def->dh_cert = virXPathString("string(./dhCert)", ctxt); > + def->session = virXPathString("string(./session)", ctxt); > + > + return g_steal_pointer(&def); > +} > + > + > +static virDomainSecDef * > +virDomainSecDefParseXML(xmlNodePtr lsecNode, > + xmlXPathContextPtr ctxt) > +{ > + g_autoptr(virDomainSecDef) sec = g_new0(virDomainSecDef, 1); > + g_autofree char *type = NULL; > > - def->policy = policy; > - def->dh_cert = virXPathString("string(./dhCert)", ctxt); > - def->session = virXPathString("string(./session)", ctxt); > + ctxt->node = lsecNode; > > - return g_steal_pointer(&def); > + if (!(type = virXMLPropString(lsecNode, "type"))) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("missing launch security type")); > + return NULL; > + } > + > + sec->sectype = virDomainLaunchSecurityTypeFromString(type); > + switch ((virDomainLaunchSecurity) sec->sectype) { > + case VIR_DOMAIN_LAUNCH_SECURITY_SEV: > + sec->sev = virDomainSEVDefParseXML(lsecNode, ctxt); > + if (!sec->sev) > + return NULL; > + break; > case VIR_DOMAIN_LAUNCH_SECURITY_NONE: > case VIR_DOMAIN_LAUNCH_SECURITY_LAST: > default: > @@ -14779,6 +14807,8 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, > type); > return NULL; > } > + > + return g_steal_pointer(&sec); > } > > > @@ -20098,10 +20128,10 @@ virDomainDefParseXML(xmlDocPtr xml, > ctxt->node = node; > VIR_FREE(nodes); > > - /* Check for SEV feature */ > + /* Check for launch security e.g. SEV feature */ > if ((node = virXPathNode("./launchSecurity", ctxt)) != NULL) { > - def->sev = virDomainSEVDefParseXML(node, ctxt); > - if (!def->sev) > + def->sec = virDomainSecDefParseXML(node, ctxt); > + if (!def->sec) > goto error; > } > > @@ -26832,15 +26862,19 @@ virDomainKeyWrapDefFormat(virBuffer *buf, virDomainKeyWrapDef *keywrap) > > > static void > -virDomainSEVDefFormat(virBuffer *buf, virDomainSEVDef *sev) > +virDomainSecDefFormat(virBuffer *buf, virDomainSecDef *sec) > { > - if (!sev) > + if (!sec) > return; > > - switch ((virDomainLaunchSecurity) sev->sectype) { > + switch ((virDomainLaunchSecurity) sec->sectype) { > case VIR_DOMAIN_LAUNCH_SECURITY_SEV: { > + virDomainSEVDef *sev = sec->sev; > + if (!sev) > + return; > + > virBufferAsprintf(buf, "<launchSecurity type='%s'>\n", > - virDomainLaunchSecurityTypeToString(sev->sectype)); > + virDomainLaunchSecurityTypeToString(sec->sectype)); > virBufferAdjustIndent(buf, 2); > > if (sev->haveCbitpos) > @@ -26860,6 +26894,7 @@ virDomainSEVDefFormat(virBuffer *buf, virDomainSEVDef *sev) > virBufferAddLit(buf, "</launchSecurity>\n"); > break; > } > + > case VIR_DOMAIN_LAUNCH_SECURITY_NONE: > case VIR_DOMAIN_LAUNCH_SECURITY_LAST: > break; > @@ -28272,7 +28307,7 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def, > if (def->keywrap) > virDomainKeyWrapDefFormat(buf, def->keywrap); > > - virDomainSEVDefFormat(buf, def->sev); > + virDomainSecDefFormat(buf, def->sec); > > if (def->namespaceData && def->ns.format) { > if ((def->ns.format)(buf, def->namespaceData) < 0) > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 512c7c8bd7..fa7ab1895d 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -2651,7 +2651,6 @@ typedef enum { > > > struct _virDomainSEVDef { > - int sectype; /* enum virDomainLaunchSecurity */ > char *dh_cert; > char *session; > unsigned int policy; > @@ -2661,6 +2660,10 @@ struct _virDomainSEVDef { > unsigned int reduced_phys_bits; > }; > > +struct _virDomainSecDef { > + int sectype; /* enum virDomainLaunchSecurity */ > + virDomainSEVDef *sev; I would rather use union here like we do in other similar internal structures: struct _virDomainSecDef { int sectype; /* enum virDomainLaunchSecurity */ union data { virDomainSEVDef sev; } } or struct _virDomainSecDef { int sectype; /* enum virDomainLaunchSecurity */ union data { virDomainSEVDef *sev; } } depending if we need to have the specific SEV structure as pointer or not based on its usage. I personally think we can do it without the pointer as it should not happen that sectype will be set to SEV but we will not have any data.

Pavel > +}; > > typedef enum { > VIR_DOMAIN_IOMMU_MODEL_INTEL, > @@ -2871,8 +2874,8 @@ struct _virDomainDef { > > virDomainKeyWrapDef *keywrap; > > - /* SEV-specific domain */ > - virDomainSEVDef *sev; > + /* launch security e.g. SEV */ > + virDomainSecDef *sec; > > /* Application-specific custom metadata */ > xmlNodePtr metadata; > @@ -3287,6 +3290,8 @@ void virDomainShmemDefFree(virDomainShmemDef *def); > G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainShmemDef, virDomainShmemDefFree); > void virDomainSEVDefFree(virDomainSEVDef *def); > G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSEVDef, virDomainSEVDefFree); > +void virDomainSecDefFree(virDomainSecDef *def); > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSecDef, virDomainSecDefFree); > void virDomainDeviceDefFree(virDomainDeviceDef *def); > > G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainDeviceDef, virDomainDeviceDefFree); > diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h > index b21068486e..21420ba8ea 100644 > --- a/src/conf/virconftypes.h > +++ b/src/conf/virconftypes.h > @@ -202,6 +202,8 @@ typedef struct _virDomainResourceDef virDomainResourceDef; > > typedef struct _virDomainSEVDef virDomainSEVDef; > > +typedef struct _virDomainSecDef virDomainSecDef; > + > typedef struct _virDomainShmemDef virDomainShmemDef; > > typedef struct _virDomainSmartcardDef virDomainSmartcardDef; > diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c > index 038d6478b2..f2d99abcfa 100644 > --- a/src/qemu/qemu_cgroup.c > +++ b/src/qemu/qemu_cgroup.c > @@ -856,7 +856,9 @@ qemuSetupDevicesCgroup(virDomainObj *vm) > return -1; > } > > - if (vm->def->sev && qemuSetupSEVCgroup(vm) < 0) > + if (vm->def->sec && > + vm->def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV && > + qemuSetupSEVCgroup(vm) < 0) > return -1; > > return 0; > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index ea513693f7..4135a8444a 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -6966,11 +6966,20 @@ qemuBuildMachineCommandLine(virCommand *cmd, > if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_LOADPARM)) > qemuAppendLoadparmMachineParm(&buf, def); > > - if (def->sev) { > - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT)) { > - virBufferAddLit(&buf, ",confidential-guest-support=sev0"); > - } else { > - virBufferAddLit(&buf, ",memory-encryption=sev0"); > + if (def->sec) { > + switch ((virDomainLaunchSecurity) def->sec->sectype) { > + case VIR_DOMAIN_LAUNCH_SECURITY_SEV: > + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT)) { > + virBufferAddLit(&buf, ",confidential-guest-support=sev0"); > + } else { > + virBufferAddLit(&buf, ",memory-encryption=sev0"); > + } > + break; > + case VIR_DOMAIN_LAUNCH_SECURITY_NONE: > + break; > + case VIR_DOMAIN_LAUNCH_SECURITY_LAST: > + virReportEnumRangeError(virDomainLaunchSecurity, def->sec->sectype); > + return -1; > } > } > > @@ -9860,6 +9869,29 @@ qemuBuildSEVCommandLine(virDomainObj *vm, virCommand *cmd, > return 0; > } > > + > +static int > +qemuBuildSecCommandLine(virDomainObj *vm, virCommand *cmd, > + virDomainSecDef *sec) > +{ > + if (!sec) > + return 0; > + > + switch ((virDomainLaunchSecurity) sec->sectype) { > + case VIR_DOMAIN_LAUNCH_SECURITY_SEV: > + return qemuBuildSEVCommandLine(vm, cmd, sec->sev); > + break; > + case VIR_DOMAIN_LAUNCH_SECURITY_NONE: > + break; > + case VIR_DOMAIN_LAUNCH_SECURITY_LAST: > + virReportEnumRangeError(virDomainLaunchSecurity, sec->sectype); > + return -1; > + } > + > + return 0; > +} > + > + > static int > qemuBuildVMCoreInfoCommandLine(virCommand *cmd, > const virDomainDef *def) > @@ -10559,7 +10591,7 @@ qemuBuildCommandLine(virQEMUDriver *driver, > if (qemuBuildVMCoreInfoCommandLine(cmd, def) < 0) > return NULL; > > - if (qemuBuildSEVCommandLine(vm, cmd, def->sev) < 0) > + if (qemuBuildSecCommandLine(vm, cmd, def->sec) < 0) > return NULL; > > if (snapshot) > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 235f575901..9973875092 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -19830,7 +19830,8 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain, > if (virDomainGetLaunchSecurityInfoEnsureACL(domain->conn, vm->def) < 0) > goto cleanup; > > - if (vm->def->sev) { > + if (vm->def->sec && > + vm->def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) { > if (qemuDomainGetSEVMeasurement(driver, vm, params, nparams, flags) < 0) > goto cleanup; > } > diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c > index e17b024b06..6d1bab181e 100644 > --- a/src/qemu/qemu_firmware.c > +++ b/src/qemu/qemu_firmware.c > @@ -1053,19 +1053,28 @@ qemuFirmwareMatchDomain(const virDomainDef *def, > return false; > } > > - if (def->sev && > - def->sev->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) { > - if (!supportsSEV) { > - VIR_DEBUG("Domain requires SEV, firmware '%s' doesn't support it", > - path); > - return false; > - } > + if (def->sec) { > + switch ((virDomainLaunchSecurity) def->sec->sectype) { > + case VIR_DOMAIN_LAUNCH_SECURITY_SEV: > + if (!supportsSEV) { > + VIR_DEBUG("Domain requires SEV, firmware '%s' doesn't support it", > + path); > + return false; > + } > > - if (def->sev->policy & VIR_QEMU_FIRMWARE_AMD_SEV_ES_POLICY && > - !supportsSEVES) { > - VIR_DEBUG("Domain requires SEV-ES, firmware '%s' doesn't support it", > - path); > - return false; > + if (def->sec->sev && > + def->sec->sev->policy & VIR_QEMU_FIRMWARE_AMD_SEV_ES_POLICY && > + !supportsSEVES) { > + VIR_DEBUG("Domain requires SEV-ES, firmware '%s' doesn't support it", > + path); > + return false; > + } > + break; > + case VIR_DOMAIN_LAUNCH_SECURITY_NONE: > + break; > + case VIR_DOMAIN_LAUNCH_SECURITY_LAST: > + virReportEnumRangeError(virDomainLaunchSecurity, def->sec->sectype); > + return -1; > } > } > > diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c > index 98495e8ef8..35c8eb83fd 100644 > --- a/src/qemu/qemu_namespace.c > +++ b/src/qemu/qemu_namespace.c > @@ -594,16 +594,26 @@ static int > qemuDomainSetupLaunchSecurity(virDomainObj *vm, > GSList **paths) > { > - virDomainSEVDef *sev = vm->def->sev; > + virDomainSecDef *sec = vm->def->sec; > > - if (!sev || sev->sectype != VIR_DOMAIN_LAUNCH_SECURITY_SEV) > + if (!sec) > return 0; > > - VIR_DEBUG("Setting up launch security"); > + switch ((virDomainLaunchSecurity) sec->sectype) { > + case VIR_DOMAIN_LAUNCH_SECURITY_SEV: > + VIR_DEBUG("Setting up launch security for SEV"); > > - *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEV_SEV)); > + *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEV_SEV)); > + > + VIR_DEBUG("Set up launch security for SEV"); > + break; > + case VIR_DOMAIN_LAUNCH_SECURITY_NONE: > + break; > + case VIR_DOMAIN_LAUNCH_SECURITY_LAST: > + virReportEnumRangeError(virDomainLaunchSecurity, sec->sectype); > + return -1; > + } > > - VIR_DEBUG("Set up launch security"); > return 0; > } > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 2b03b0ab98..d9073fb3a3 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -6480,7 +6480,7 @@ qemuProcessUpdateSEVInfo(virDomainObj *vm) > { > qemuDomainObjPrivate *priv = vm->privateData; > virQEMUCaps *qemuCaps = priv->qemuCaps; > - virDomainSEVDef *sev = vm->def->sev; > + virDomainSEVDef *sev = vm->def->sec->sev; > virSEVCapability *sevCaps = NULL; > > /* if platform specific info like 'cbitpos' and 'reducedPhysBits' have > @@ -6636,7 +6636,8 @@ qemuProcessPrepareDomain(virQEMUDriver *driver, > for (i = 0; i < vm->def->nshmems; i++) > qemuDomainPrepareShmemChardev(vm->def->shmems[i]); > > - if (vm->def->sev) { > + if (vm->def->sec && > + vm->def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) { > VIR_DEBUG("Updating SEV platform info"); > if (qemuProcessUpdateSEVInfo(vm) < 0) > return -1; > @@ -6674,10 +6675,10 @@ qemuProcessSEVCreateFile(virDomainObj *vm, > static int > qemuProcessPrepareSEVGuestInput(virDomainObj *vm) > { > - virDomainSEVDef *sev = vm->def->sev; > + virDomainSEVDef *sev = vm->def->sec->sev; > > if (!sev) > - return 0; > + return -1; This should not happen as we would abort if allocation of virDomainSEVDef failed. In addition if we go with the union where the data would not be a pointer there is no need for this check at all.

Pavel > VIR_DEBUG("Preparing SEV guest"); > > @@ -6695,6 +6696,28 @@ qemuProcessPrepareSEVGuestInput(virDomainObj *vm) > } > > > +static int > +qemuProcessPrepareLaunchSecurityGuestInput(virDomainObj *vm) > +{ > + virDomainSecDef *sec = vm->def->sec; > + > + if (!sec) > + return 0; > + > + switch ((virDomainLaunchSecurity) sec->sectype) { > + case VIR_DOMAIN_LAUNCH_SECURITY_SEV: > + return qemuProcessPrepareSEVGuestInput(vm); > + case VIR_DOMAIN_LAUNCH_SECURITY_NONE: > + break; > + case VIR_DOMAIN_LAUNCH_SECURITY_LAST: > + virReportEnumRangeError(virDomainLaunchSecurity, sec->sectype); > + return -1; > + } > + > + return 0; > +} > + > + > static int > qemuProcessPrepareHostStorage(virQEMUDriver *driver, > virDomainObj *vm, > @@ -6874,7 +6897,7 @@ qemuProcessPrepareHost(virQEMUDriver *driver, > if (qemuExtDevicesPrepareHost(driver, vm) < 0) > return -1; > > - if (qemuProcessPrepareSEVGuestInput(vm) < 0) > + if (qemuProcessPrepareLaunchSecurityGuestInput(vm) < 0) > return -1; > > return 0; > diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c > index 382473d03b..957dbc906c 100644 > --- a/src/qemu/qemu_validate.c > +++ b/src/qemu/qemu_validate.c > @@ -1214,12 +1214,22 @@ qemuValidateDomainDef(const virDomainDef *def, > if (qemuValidateDomainDefPanic(def, qemuCaps) < 0) > return -1; > > - if (def->sev && > - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) { > - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > - _("SEV launch security is not supported with " > - "this QEMU binary")); > - return -1; > + if (def->sec) { > + switch ((virDomainLaunchSecurity) def->sec->sectype) { > + case VIR_DOMAIN_LAUNCH_SECURITY_SEV: > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("SEV launch security is not supported with " > + "this QEMU binary")); > + return -1; > + } > + break; > + case VIR_DOMAIN_LAUNCH_SECURITY_NONE: > + break; > + case VIR_DOMAIN_LAUNCH_SECURITY_LAST: > + virReportEnumRangeError(virDomainLaunchSecurity, def->sec->sectype); > + return -1; > + } > } > > if (def->naudios > 1 && > diff --git a/src/security/security_dac.c b/src/security/security_dac.c > index 4909107fcc..b874dd4ab6 100644 > --- a/src/security/security_dac.c > +++ b/src/security/security_dac.c > @@ -1958,7 +1958,8 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr, > rc = -1; > } > > - if (def->sev) { > + if (def->sec && > + def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) { > if (virSecurityDACRestoreSEVLabel(mgr, def) < 0) > rc = -1; > } > @@ -2165,7 +2166,8 @@ virSecurityDACSetAllLabel(virSecurityManager *mgr, > return -1; > } > > - if (def->sev) { > + if (def->sec && > + def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV) { > if (virSecurityDACSetSEVLabel(mgr, def) < 0) > return -1; > } > -- > 2.30.2 >