On 6/25/21 10:51 AM, Pavel Hrdina wrote:
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@xxxxxxxxxxxxx> --- 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.
OK, will change it.
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.cindex 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.cindex 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.
OK, I will remove it.
PavelVIR_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
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
