[PATCH 0/8] conf: Don't lose <active_pcr_banks/> when no TPM version is provided
*** BLURB HERE *** Michal Prívozník (8): conf: Report an error when default TPM model is provided conf: Report error when default TPM version is provided conf: Drop needless setting of VIR_DOMAIN_TPM_VERSION_DEFAULT conf: Move _virDomainTPMDef::version into _virDomainTPMDef::data::emulator conf: Use virXMLPropEnum more when parsing TPM qemu_domain: Move TPM post parse code into qemuDomainTPMDefPostParse() qemu: Move TPMs validation out of PostParse conf: Don't lose <active_pcr_banks/> when no TPM version is provided src/conf/domain_conf.c | 81 +++++++++++++------------------- src/conf/domain_conf.h | 10 ++-- src/conf/domain_validate.c | 28 ++++++++++- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 63 ++++++------------------- src/qemu/qemu_tpm.c | 10 ++-- src/qemu/qemu_validate.c | 87 ++++++++++++++++++++++++----------- src/security/virt-aa-helper.c | 2 +- 8 files changed, 147 insertions(+), 136 deletions(-) -- 2.35.1
When "default" model of a TPM was provided, our parses accepts it happily even though the value is forbidden by our RNG and not documented as accepted value. This is because of < 0 vs <= 0 comparison of virDomainTPMModelTypeFromString() retval. Make the parser error out explicitly in this case. Users can always chose to not specify the attribute in which case we pick a sane default (in qemuDomainTPMDefPostParse()). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4c7a5a044c..b7147945da 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10360,7 +10360,7 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, model = virXMLPropString(node, "model"); if (model != NULL && - (def->model = virDomainTPMModelTypeFromString(model)) < 0) { + (def->model = virDomainTPMModelTypeFromString(model)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown TPM frontend model '%s'"), model); goto error; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 90de50c12f..5a057c36b8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1400,7 +1400,7 @@ struct _virDomainHubDef { }; typedef enum { - VIR_DOMAIN_TPM_MODEL_DEFAULT, + VIR_DOMAIN_TPM_MODEL_DEFAULT = 0, VIR_DOMAIN_TPM_MODEL_TIS, VIR_DOMAIN_TPM_MODEL_CRB, VIR_DOMAIN_TPM_MODEL_SPAPR, -- 2.35.1
On Mon, Jul 18, 2022 at 11:30:43 +0200, Michal Privoznik wrote:
When "default" model of a TPM was provided, our parses accepts it happily even though the value is forbidden by our RNG and not documented as accepted value. This is because of < 0 vs <= 0 comparison of virDomainTPMModelTypeFromString() retval.
Make the parser error out explicitly in this case. Users can always chose to not specify the attribute in which case we pick a sane default (in qemuDomainTPMDefPostParse()).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4c7a5a044c..b7147945da 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10360,7 +10360,7 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt,
model = virXMLPropString(node, "model"); if (model != NULL && - (def->model = virDomainTPMModelTypeFromString(model)) < 0) { + (def->model = virDomainTPMModelTypeFromString(model)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown TPM frontend model '%s'"), model); goto error;
'virDomainTPMDefFormat' happily formats 'default' as supported type: virBufferAsprintf(&attrBuf, " model='%s'", virDomainTPMModelTypeToString(def->model)); Is there any other code path which would forbid 'default'? If no, then we might run into a situation where libvirt's parser would reject parsing a XML formatted by libvirt itself, which is not acceptable. In such case we'd need to leave the parser as-is and add just validation where 'defau't will be forbidden, which is acceptable.
On 8/1/22 13:10, Peter Krempa wrote:
On Mon, Jul 18, 2022 at 11:30:43 +0200, Michal Privoznik wrote:
When "default" model of a TPM was provided, our parses accepts it happily even though the value is forbidden by our RNG and not documented as accepted value. This is because of < 0 vs <= 0 comparison of virDomainTPMModelTypeFromString() retval.
Make the parser error out explicitly in this case. Users can always chose to not specify the attribute in which case we pick a sane default (in qemuDomainTPMDefPostParse()).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4c7a5a044c..b7147945da 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10360,7 +10360,7 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt,
model = virXMLPropString(node, "model"); if (model != NULL && - (def->model = virDomainTPMModelTypeFromString(model)) < 0) { + (def->model = virDomainTPMModelTypeFromString(model)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown TPM frontend model '%s'"), model); goto error;
'virDomainTPMDefFormat' happily formats 'default' as supported type:
virBufferAsprintf(&attrBuf, " model='%s'", virDomainTPMModelTypeToString(def->model));
Is there any other code path which would forbid 'default'?
Couple of them, actually. The first one is in qemuValidateDomainDeviceDefTPM() where virQEMUCapsFillDomainDeviceTPMCaps(qemuCaps, &tpmCaps); is called followed by if (!VIR_DOMAIN_CAPS_ENUM_IS_SET(tpmCaps.model, tpm->model)) {}. And the second is qemuDomainTPMDefPostParse() which overwrites the _DEFAULT to either _TIS or _SPAPR (alright, this is not a check that forbids 'default' per se).
If no, then we might run into a situation where libvirt's parser would reject parsing a XML formatted by libvirt itself, which is not acceptable.
In such case we'd need to leave the parser as-is and add just validation where 'defau't will be forbidden, which is acceptable.
Michal
On Mon, Aug 01, 2022 at 15:08:23 +0200, Michal Prívozník wrote:
On 8/1/22 13:10, Peter Krempa wrote:
On Mon, Jul 18, 2022 at 11:30:43 +0200, Michal Privoznik wrote:
When "default" model of a TPM was provided, our parses accepts it happily even though the value is forbidden by our RNG and not documented as accepted value. This is because of < 0 vs <= 0 comparison of virDomainTPMModelTypeFromString() retval.
Make the parser error out explicitly in this case. Users can always chose to not specify the attribute in which case we pick a sane default (in qemuDomainTPMDefPostParse()).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4c7a5a044c..b7147945da 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10360,7 +10360,7 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt,
model = virXMLPropString(node, "model"); if (model != NULL && - (def->model = virDomainTPMModelTypeFromString(model)) < 0) { + (def->model = virDomainTPMModelTypeFromString(model)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown TPM frontend model '%s'"), model); goto error;
'virDomainTPMDefFormat' happily formats 'default' as supported type:
virBufferAsprintf(&attrBuf, " model='%s'", virDomainTPMModelTypeToString(def->model));
Is there any other code path which would forbid 'default'?
Couple of them, actually. The first one is in qemuValidateDomainDeviceDefTPM() where virQEMUCapsFillDomainDeviceTPMCaps(qemuCaps, &tpmCaps); is called followed by if (!VIR_DOMAIN_CAPS_ENUM_IS_SET(tpmCaps.model, tpm->model)) {}. And the second is qemuDomainTPMDefPostParse() which overwrites the _DEFAULT to either _TIS or _SPAPR (alright, this is not a check that forbids 'default' per se).
Taken very strictly this would not apply for other drivers though, which in most cases don't even reject TPM devices, but that would be a very theoretical excercise. If you want to go ahead and make the parser more strict, which should be most likely okay in any reasonable case, you should in such case modify the formatter to skip the _DEFAULT values so they don't end up in the XML accidentally and then make it un-parsable.
On 8/1/22 16:29, Peter Krempa wrote:
On Mon, Aug 01, 2022 at 15:08:23 +0200, Michal Prívozník wrote:
On 8/1/22 13:10, Peter Krempa wrote:
On Mon, Jul 18, 2022 at 11:30:43 +0200, Michal Privoznik wrote:
When "default" model of a TPM was provided, our parses accepts it happily even though the value is forbidden by our RNG and not documented as accepted value. This is because of < 0 vs <= 0 comparison of virDomainTPMModelTypeFromString() retval.
Make the parser error out explicitly in this case. Users can always chose to not specify the attribute in which case we pick a sane default (in qemuDomainTPMDefPostParse()).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4c7a5a044c..b7147945da 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10360,7 +10360,7 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt,
model = virXMLPropString(node, "model"); if (model != NULL && - (def->model = virDomainTPMModelTypeFromString(model)) < 0) { + (def->model = virDomainTPMModelTypeFromString(model)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown TPM frontend model '%s'"), model); goto error;
'virDomainTPMDefFormat' happily formats 'default' as supported type:
virBufferAsprintf(&attrBuf, " model='%s'", virDomainTPMModelTypeToString(def->model));
Is there any other code path which would forbid 'default'?
Couple of them, actually. The first one is in qemuValidateDomainDeviceDefTPM() where virQEMUCapsFillDomainDeviceTPMCaps(qemuCaps, &tpmCaps); is called followed by if (!VIR_DOMAIN_CAPS_ENUM_IS_SET(tpmCaps.model, tpm->model)) {}. And the second is qemuDomainTPMDefPostParse() which overwrites the _DEFAULT to either _TIS or _SPAPR (alright, this is not a check that forbids 'default' per se).
Taken very strictly this would not apply for other drivers though, which in most cases don't even reject TPM devices, but that would be a very theoretical excercise.
True, no other driver rejects TPM, but at the same time no other driver supports it. I believe our policy was to do nothing in such case. We tell users to try out their XML first to see whether a device is supported. TPM is not alone in this.
If you want to go ahead and make the parser more strict, which should be most likely okay in any reasonable case, you should in such case modify the formatter to skip the _DEFAULT values so they don't end up in the XML accidentally and then make it un-parsable.
Well, if _DEFAULT model (or _DEFAULT version from our discussion to 2/8) was provided then already users would have to specifically ignore schema validation AND our documentation. But yes, it is possible to define the following XML: <tpm model='default'> <backend type='emulator' version='default'/> </tpm> for say the test driver, which lacks those default -> sensible value post parse callbacks. In this case we would format XML and won't be able to parse it back. Except for QEMU driver. But okay, consider the following squashed into their respective commits: diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c index 83db800f51..1647692ea2 100644 --- i/src/conf/domain_conf.c +++ w/src/conf/domain_conf.c @@ -24230,8 +24230,10 @@ virDomainTPMDefFormat(virBuffer *buf, g_auto(virBuffer) backendAttrBuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) backendChildBuf = VIR_BUFFER_INIT_CHILD(&childBuf); - virBufferAsprintf(&attrBuf, " model='%s'", - virDomainTPMModelTypeToString(def->model)); + if (def->model) { + virBufferAsprintf(&attrBuf, " model='%s'", + virDomainTPMModelTypeToString(def->model)); + } virBufferAsprintf(&backendAttrBuf, " type='%s'", virDomainTPMBackendTypeToString(def->type)); @@ -24242,8 +24244,10 @@ virDomainTPMDefFormat(virBuffer *buf, def->data.passthrough.source->data.file.path); break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: - virBufferAsprintf(&backendAttrBuf, " version='%s'", - virDomainTPMVersionTypeToString(def->version)); + if (def->version) { + virBufferAsprintf(&backendAttrBuf, " version='%s'", + virDomainTPMVersionTypeToString(def->version)); + } if (def->data.emulator.persistent_state) virBufferAddLit(&backendAttrBuf, " persistent_state='yes'"); if (def->data.emulator.hassecretuuid) { Michal
On Mon, Aug 01, 2022 at 16:59:26 +0200, Michal Prívozník wrote:
On 8/1/22 16:29, Peter Krempa wrote:
On Mon, Aug 01, 2022 at 15:08:23 +0200, Michal Prívozník wrote:
On 8/1/22 13:10, Peter Krempa wrote:
On Mon, Jul 18, 2022 at 11:30:43 +0200, Michal Privoznik wrote:
When "default" model of a TPM was provided, our parses accepts it happily even though the value is forbidden by our RNG and not documented as accepted value. This is because of < 0 vs <= 0 comparison of virDomainTPMModelTypeFromString() retval.
Make the parser error out explicitly in this case. Users can always chose to not specify the attribute in which case we pick a sane default (in qemuDomainTPMDefPostParse()).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4c7a5a044c..b7147945da 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10360,7 +10360,7 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt,
model = virXMLPropString(node, "model"); if (model != NULL && - (def->model = virDomainTPMModelTypeFromString(model)) < 0) { + (def->model = virDomainTPMModelTypeFromString(model)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown TPM frontend model '%s'"), model); goto error;
'virDomainTPMDefFormat' happily formats 'default' as supported type:
virBufferAsprintf(&attrBuf, " model='%s'", virDomainTPMModelTypeToString(def->model));
Is there any other code path which would forbid 'default'?
Couple of them, actually. The first one is in qemuValidateDomainDeviceDefTPM() where virQEMUCapsFillDomainDeviceTPMCaps(qemuCaps, &tpmCaps); is called followed by if (!VIR_DOMAIN_CAPS_ENUM_IS_SET(tpmCaps.model, tpm->model)) {}. And the second is qemuDomainTPMDefPostParse() which overwrites the _DEFAULT to either _TIS or _SPAPR (alright, this is not a check that forbids 'default' per se).
Taken very strictly this would not apply for other drivers though, which in most cases don't even reject TPM devices, but that would be a very theoretical excercise.
True, no other driver rejects TPM, but at the same time no other driver supports it. I believe our policy was to do nothing in such case. We tell users to try out their XML first to see whether a device is supported. TPM is not alone in this.
Sure, but in many cases where we reject the default at parsing we also make sure to prevent programming errors from re-parsing the XML as that can be sub-optimal for the health of a running VM. In this particular instance you are changing existing code so obviously it falls under more scrutiny.
If you want to go ahead and make the parser more strict, which should be most likely okay in any reasonable case, you should in such case modify the formatter to skip the _DEFAULT values so they don't end up in the XML accidentally and then make it un-parsable.
Well, if _DEFAULT model (or _DEFAULT version from our discussion to 2/8) was provided then already users would have to specifically ignore schema validation AND our documentation. But yes, it is possible to define the following XML:
<tpm model='default'> <backend type='emulator' version='default'/> </tpm>
for say the test driver, which lacks those default -> sensible value post parse callbacks. In this case we would format XML and won't be able to parse it back. Except for QEMU driver.
Apart from the gross user negligence above, we can have cases where a device definition is mistakenly allocated and doesn't undergo defaults filling for some reason, which would create a XML we refuse to parse.
But okay, consider the following squashed into their respective commits:
diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c index 83db800f51..1647692ea2 100644 --- i/src/conf/domain_conf.c +++ w/src/conf/domain_conf.c @@ -24230,8 +24230,10 @@ virDomainTPMDefFormat(virBuffer *buf, g_auto(virBuffer) backendAttrBuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) backendChildBuf = VIR_BUFFER_INIT_CHILD(&childBuf);
- virBufferAsprintf(&attrBuf, " model='%s'", - virDomainTPMModelTypeToString(def->model)); + if (def->model) { + virBufferAsprintf(&attrBuf, " model='%s'", + virDomainTPMModelTypeToString(def->model)); + }
virBufferAsprintf(&backendAttrBuf, " type='%s'", virDomainTPMBackendTypeToString(def->type)); @@ -24242,8 +24244,10 @@ virDomainTPMDefFormat(virBuffer *buf, def->data.passthrough.source->data.file.path); break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: - virBufferAsprintf(&backendAttrBuf, " version='%s'", - virDomainTPMVersionTypeToString(def->version)); + if (def->version) { + virBufferAsprintf(&backendAttrBuf, " version='%s'", + virDomainTPMVersionTypeToString(def->version)); + } if (def->data.emulator.persistent_state) virBufferAddLit(&backendAttrBuf, " persistent_state='yes'"); if (def->data.emulator.hassecretuuid) {
Preferrably use an explicit comparison against the appropriate _DEFAULT. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
When "default" version of TPM was provided, our parses accepts it happily even though the value is forbidden by our RNG and not documented as accepted value. This is because of < 0 vs <= 0 comparison of virDomainTPMModelTypeFromString() retval. Make the parser error out explicitly in this case. Users can always chose to not specify the attribute in which case we pick a sane default (in qemuDomainDefTPMsPostParse()). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b7147945da..6c178783af 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10400,7 +10400,7 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, if (!version) { def->version = VIR_DOMAIN_TPM_VERSION_DEFAULT; } else { - if ((def->version = virDomainTPMVersionTypeFromString(version)) < 0) { + if ((def->version = virDomainTPMVersionTypeFromString(version)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unsupported TPM version '%s'"), version); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 5a057c36b8..7139b91aca 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1417,7 +1417,7 @@ typedef enum { } virDomainTPMBackendType; typedef enum { - VIR_DOMAIN_TPM_VERSION_DEFAULT, + VIR_DOMAIN_TPM_VERSION_DEFAULT = 0, VIR_DOMAIN_TPM_VERSION_1_2, VIR_DOMAIN_TPM_VERSION_2_0, -- 2.35.1
On Mon, Jul 18, 2022 at 11:30:44 +0200, Michal Privoznik wrote:
When "default" version of TPM was provided, our parses accepts it happily even though the value is forbidden by our RNG and not documented as accepted value. This is because of < 0 vs <= 0 comparison of virDomainTPMModelTypeFromString() retval.
Make the parser error out explicitly in this case. Users can always chose to not specify the attribute in which case we pick a sane default (in qemuDomainDefTPMsPostParse()).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
This has the same issue as I've reported in previous patch.
On 8/1/22 13:11, Peter Krempa wrote:
On Mon, Jul 18, 2022 at 11:30:44 +0200, Michal Privoznik wrote:
When "default" version of TPM was provided, our parses accepts it happily even though the value is forbidden by our RNG and not documented as accepted value. This is because of < 0 vs <= 0 comparison of virDomainTPMModelTypeFromString() retval.
Make the parser error out explicitly in this case. Users can always chose to not specify the attribute in which case we pick a sane default (in qemuDomainDefTPMsPostParse()).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
This has the same issue as I've reported in previous patch.
The 'default' gets overwritten in qemuDomainDefTPMsPostParse() to something sensible. So we would never ever output 'default'. Michal
On Mon, Aug 01, 2022 at 15:08:27 +0200, Michal Prívozník wrote:
On 8/1/22 13:11, Peter Krempa wrote:
On Mon, Jul 18, 2022 at 11:30:44 +0200, Michal Privoznik wrote:
When "default" version of TPM was provided, our parses accepts it happily even though the value is forbidden by our RNG and not documented as accepted value. This is because of < 0 vs <= 0 comparison of virDomainTPMModelTypeFromString() retval.
Make the parser error out explicitly in this case. Users can always chose to not specify the attribute in which case we pick a sane default (in qemuDomainDefTPMsPostParse()).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
This has the same issue as I've reported in previous patch.
The 'default' gets overwritten in qemuDomainDefTPMsPostParse() to something sensible. So we would never ever output 'default'.
Thus this boils down to same condition I gave with previous patch. If you want to make the parser more strict, make the formatter skip the value which would be rejected by the parser.
In previous commit the VIR_DOMAIN_TPM_VERSION_DEFAULT value was made just an alias to value of 0. And since all newly allocated memory is zeroed out (due to use of g_new0()), the def->version inside of virDomainTPMDefParseXML() is also 0 and thus there is no need to set it explicitly. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6c178783af..2d8989e4ff 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10397,15 +10397,12 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, } version = virXMLPropString(backends[0], "version"); - if (!version) { - def->version = VIR_DOMAIN_TPM_VERSION_DEFAULT; - } else { - if ((def->version = virDomainTPMVersionTypeFromString(version)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unsupported TPM version '%s'"), - version); - goto error; - } + if (version && + (def->version = virDomainTPMVersionTypeFromString(version)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported TPM version '%s'"), + version); + goto error; } switch (def->type) { -- 2.35.1
On Mon, Jul 18, 2022 at 11:30:45 +0200, Michal Privoznik wrote:
In previous commit the VIR_DOMAIN_TPM_VERSION_DEFAULT value was made just an alias to value of 0.
This is true even now.
And since all newly allocated memory is zeroed out (due to use of g_new0()), the def->version inside of virDomainTPMDefParseXML() is also 0 and thus there is no need to set it explicitly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6c178783af..2d8989e4ff 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10397,15 +10397,12 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, }
version = virXMLPropString(backends[0], "version"); - if (!version) { - def->version = VIR_DOMAIN_TPM_VERSION_DEFAULT; - } else { - if ((def->version = virDomainTPMVersionTypeFromString(version)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unsupported TPM version '%s'"), - version); - goto error; - } + if (version && + (def->version = virDomainTPMVersionTypeFromString(version)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported TPM version '%s'"), + version);
You can use: Reviewed-by: Peter Krempa <pkrempa@redhat.com> With appropriate adaptation if previous patch ends up to be modified.
The _virDomainTPMDef structure has 'version' member, which is a bit misplaced. It's only emulator type of TPM that can have a version, even our documentation says so: ``version`` The ``version`` attribute indicates the version of the TPM. This attribute only works with the ``emulator`` backend. The following versions are supported: Therefore, move the member into that part of union that's covering emulated TPM devices. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 34 +++++++++++----------- src/conf/domain_conf.h | 2 +- src/qemu/qemu_domain.c | 7 +++-- src/qemu/qemu_tpm.c | 10 ++++--- src/qemu/qemu_validate.c | 53 ++++++++++++++++++----------------- src/security/virt-aa-helper.c | 2 +- 6 files changed, 56 insertions(+), 52 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2d8989e4ff..28f0e75e60 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10396,15 +10396,6 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, goto error; } - version = virXMLPropString(backends[0], "version"); - if (version && - (def->version = virDomainTPMVersionTypeFromString(version)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unsupported TPM version '%s'"), - version); - goto error; - } - switch (def->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: if (!(def->data.passthrough.source = virDomainChrSourceDefNew(xmlopt))) @@ -10416,6 +10407,15 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, def->data.passthrough.source->data.file.path = g_steal_pointer(&path); break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: + version = virXMLPropString(backends[0], "version"); + if (version && + (def->data.emulator.version = virDomainTPMVersionTypeFromString(version)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported TPM version '%s'"), + version); + goto error; + } + if (!(def->data.emulator.source = virDomainChrSourceDefNew(xmlopt))) goto error; secretuuid = virXPathString("string(./backend/encryption/@secret)", ctxt); @@ -10437,7 +10437,7 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, goto error; } } - if (def->version == VIR_DOMAIN_TPM_VERSION_2_0) { + if (def->data.emulator.version == VIR_DOMAIN_TPM_VERSION_2_0) { if ((nnodes = virXPathNodeSet("./backend/active_pcr_banks/*", ctxt, &nodes)) < 0) break; for (i = 0; i < nnodes; i++) { @@ -20658,14 +20658,14 @@ virDomainTPMDefCheckABIStability(virDomainTPMDef *src, return false; } - if (src->version != dst->version) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Target TPM version doesn't match source")); - return false; - } - switch (src->type) { case VIR_DOMAIN_TPM_TYPE_EMULATOR: + if (src->data.emulator.version != dst->data.emulator.version) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Target TPM version doesn't match source")); + return false; + } + if (src->data.emulator.activePcrBanks != dst->data.emulator.activePcrBanks) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Target active PCR banks doesn't match source")); @@ -24219,7 +24219,7 @@ virDomainTPMDefFormat(virBuffer *buf, break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: virBufferAsprintf(&backendAttrBuf, " version='%s'", - virDomainTPMVersionTypeToString(def->version)); + virDomainTPMVersionTypeToString(def->data.emulator.version)); if (def->data.emulator.persistent_state) virBufferAddLit(&backendAttrBuf, " persistent_state='yes'"); if (def->data.emulator.hassecretuuid) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 7139b91aca..3362042db5 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1439,12 +1439,12 @@ struct _virDomainTPMDef { int type; /* virDomainTPMBackendType */ virDomainDeviceInfo info; int model; /* virDomainTPMModel */ - int version; /* virDomainTPMVersion */ union { struct { virDomainChrSourceDef *source; } passthrough; struct { + int version; /* virDomainTPMVersion */ virDomainChrSourceDef *source; char *storagepath; char *logfile; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 94b2e3118c..0343fd3597 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4594,13 +4594,14 @@ qemuDomainDefTPMsPostParse(virDomainDef *def) virDomainTPMDef *tpm = def->tpms[i]; /* TPM 1.2 and 2 are not compatible, so we choose a specific version here */ - if (tpm->version == VIR_DOMAIN_TPM_VERSION_DEFAULT) { + if (tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR && + tpm->data.emulator.version == VIR_DOMAIN_TPM_VERSION_DEFAULT) { if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR || tpm->model == VIR_DOMAIN_TPM_MODEL_CRB || qemuDomainIsARMVirt(def)) - tpm->version = VIR_DOMAIN_TPM_VERSION_2_0; + tpm->data.emulator.version = VIR_DOMAIN_TPM_VERSION_2_0; else - tpm->version = VIR_DOMAIN_TPM_VERSION_1_2; + tpm->data.emulator.version = VIR_DOMAIN_TPM_VERSION_1_2; } if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY) { diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 03829775b8..f28dd2e1e9 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -575,7 +575,8 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, if (created && qemuTPMEmulatorRunSetup(tpm->data.emulator.storagepath, vmname, vmuuid, privileged, swtpm_user, swtpm_group, - tpm->data.emulator.logfile, tpm->version, + tpm->data.emulator.logfile, + tpm->data.emulator.version, secretuuid, incomingMigration) < 0) goto error; @@ -583,7 +584,8 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, qemuTPMEmulatorReconfigure(tpm->data.emulator.storagepath, swtpm_user, swtpm_group, tpm->data.emulator.activePcrBanks, - tpm->data.emulator.logfile, tpm->version, + tpm->data.emulator.logfile, + tpm->data.emulator.version, secretuuid) < 0) goto error; @@ -611,7 +613,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, virCommandSetUID(cmd, swtpm_user); virCommandSetGID(cmd, swtpm_group); - switch (tpm->version) { + switch (tpm->data.emulator.version) { case VIR_DOMAIN_TPM_VERSION_1_2: break; case VIR_DOMAIN_TPM_VERSION_2_0: @@ -684,7 +686,7 @@ qemuTPMEmulatorInitPaths(virDomainTPMDef *tpm, if (!tpm->data.emulator.storagepath && !(tpm->data.emulator.storagepath = qemuTPMEmulatorStorageBuildPath(swtpmStorageDir, uuidstr, - tpm->version))) + tpm->data.emulator.version))) return -1; if (!tpm->data.emulator.logfile) { diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 764d5b029e..ff164118b7 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -4760,33 +4760,34 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm, { virDomainCapsDeviceTPM tpmCaps = { 0 }; - switch (tpm->version) { - case VIR_DOMAIN_TPM_VERSION_1_2: - /* TPM 1.2 + CRB do not work */ - if (tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR && - tpm->model == VIR_DOMAIN_TPM_MODEL_CRB) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unsupported interface %s for TPM 1.2"), - virDomainTPMModelTypeToString(tpm->model)); - return -1; + if (tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR) { + switch (tpm->data.emulator.version) { + case VIR_DOMAIN_TPM_VERSION_1_2: + /* TPM 1.2 + CRB do not work */ + if (tpm->model == VIR_DOMAIN_TPM_MODEL_CRB) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported interface %s for TPM 1.2"), + virDomainTPMModelTypeToString(tpm->model)); + return -1; + } + /* TPM 1.2 + SPAPR do not work with any 'type' (backend) */ + if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("TPM 1.2 is not supported with the SPAPR device model")); + return -1; + } + /* TPM 1.2 + ARM does not work */ + if (qemuDomainIsARMVirt(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("TPM 1.2 is not supported on ARM")); + return -1; + } + break; + case VIR_DOMAIN_TPM_VERSION_2_0: + case VIR_DOMAIN_TPM_VERSION_DEFAULT: + case VIR_DOMAIN_TPM_VERSION_LAST: + break; } - /* TPM 1.2 + SPAPR do not work with any 'type' (backend) */ - if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("TPM 1.2 is not supported with the SPAPR device model")); - return -1; - } - /* TPM 1.2 + ARM does not work */ - if (qemuDomainIsARMVirt(def)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("TPM 1.2 is not supported on ARM")); - return -1; - } - break; - case VIR_DOMAIN_TPM_VERSION_2_0: - case VIR_DOMAIN_TPM_VERSION_DEFAULT: - case VIR_DOMAIN_TPM_VERSION_LAST: - break; } virQEMUCapsFillDomainDeviceTPMCaps(qemuCaps, &tpmCaps); diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 8629503e11..2d0bc99c73 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1212,7 +1212,7 @@ get_files(vahControl * ctl) shortName = virDomainDefGetShortName(ctl->def); - switch (ctl->def->tpms[i]->version) { + switch (ctl->def->tpms[i]->data.emulator.version) { case VIR_DOMAIN_TPM_VERSION_1_2: tpmpath = "tpm1.2"; break; -- 2.35.1
On Mon, Jul 18, 2022 at 11:30:46 +0200, Michal Privoznik wrote:
The _virDomainTPMDef structure has 'version' member, which is a bit misplaced. It's only emulator type of TPM that can have a version, even our documentation says so:
``version`` The ``version`` attribute indicates the version of the TPM. This attribute only works with the ``emulator`` backend. The following versions are supported:
Therefore, move the member into that part of union that's covering emulated TPM devices.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 34 +++++++++++----------- src/conf/domain_conf.h | 2 +- src/qemu/qemu_domain.c | 7 +++-- src/qemu/qemu_tpm.c | 10 ++++--- src/qemu/qemu_validate.c | 53 ++++++++++++++++++----------------- src/security/virt-aa-helper.c | 2 +- 6 files changed, 56 insertions(+), 52 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2d8989e4ff..28f0e75e60 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10396,15 +10396,6 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, goto error; }
- version = virXMLPropString(backends[0], "version"); - if (version && - (def->version = virDomainTPMVersionTypeFromString(version)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unsupported TPM version '%s'"), - version); - goto error; - } - switch (def->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: if (!(def->data.passthrough.source = virDomainChrSourceDefNew(xmlopt))) @@ -10416,6 +10407,15 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, def->data.passthrough.source->data.file.path = g_steal_pointer(&path); break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: + version = virXMLPropString(backends[0], "version"); + if (version && + (def->data.emulator.version = virDomainTPMVersionTypeFromString(version)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported TPM version '%s'"), + version); + goto error; + } + if (!(def->data.emulator.source = virDomainChrSourceDefNew(xmlopt))) goto error; secretuuid = virXPathString("string(./backend/encryption/@secret)", ctxt);
Same conditions for using my R-b as per previous patch apply. [...]
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 764d5b029e..ff164118b7 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -4760,33 +4760,34 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm, { virDomainCapsDeviceTPM tpmCaps = { 0 };
- switch (tpm->version) { - case VIR_DOMAIN_TPM_VERSION_1_2: - /* TPM 1.2 + CRB do not work */ - if (tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR && - tpm->model == VIR_DOMAIN_TPM_MODEL_CRB) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unsupported interface %s for TPM 1.2"), - virDomainTPMModelTypeToString(tpm->model)); - return -1; + if (tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR) { + switch (tpm->data.emulator.version) { + case VIR_DOMAIN_TPM_VERSION_1_2: + /* TPM 1.2 + CRB do not work */ + if (tpm->model == VIR_DOMAIN_TPM_MODEL_CRB) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported interface %s for TPM 1.2"), + virDomainTPMModelTypeToString(tpm->model));
I know this is supposed to be a pure code movement but please add quotes around '%s' in the error message.
+ return -1; + } + /* TPM 1.2 + SPAPR do not work with any 'type' (backend) */ + if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("TPM 1.2 is not supported with the SPAPR device model")); + return -1;
Under same conditions as previous patch: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
When parsing a TPM device plenty of virXMLPropString() + enum2int() combos are used. These can be replaced with virXMLPropEnum(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 39 ++++++++++++--------------------------- src/conf/domain_conf.h | 6 +++--- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 2 +- 4 files changed, 17 insertions(+), 32 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 28f0e75e60..6263d90fdb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10347,9 +10347,6 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, int nnodes; size_t i; g_autofree char *path = NULL; - g_autofree char *model = NULL; - g_autofree char *backend = NULL; - g_autofree char *version = NULL; g_autofree char *secretuuid = NULL; g_autofree char *persistent_state = NULL; g_autofree xmlNodePtr *backends = NULL; @@ -10358,13 +10355,11 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, def = g_new0(virDomainTPMDef, 1); - model = virXMLPropString(node, "model"); - if (model != NULL && - (def->model = virDomainTPMModelTypeFromString(model)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unknown TPM frontend model '%s'"), model); + if (virXMLPropEnum(node, "model", + virDomainTPMModelTypeFromString, + VIR_XML_PROP_NONZERO, + &def->model) < 0) goto error; - } ctxt->node = node; @@ -10383,18 +10378,11 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, goto error; } - if (!(backend = virXMLPropString(backends[0], "type"))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing TPM device backend type")); + if (virXMLPropEnum(backends[0], "type", + virDomainTPMBackendTypeFromString, + VIR_XML_PROP_REQUIRED, + &def->type) < 0) goto error; - } - - if ((def->type = virDomainTPMBackendTypeFromString(backend)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unknown TPM backend type '%s'"), - backend); - goto error; - } switch (def->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: @@ -10407,14 +10395,11 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, def->data.passthrough.source->data.file.path = g_steal_pointer(&path); break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: - version = virXMLPropString(backends[0], "version"); - if (version && - (def->data.emulator.version = virDomainTPMVersionTypeFromString(version)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unsupported TPM version '%s'"), - version); + if (virXMLPropEnum(backends[0], "version", + virDomainTPMVersionTypeFromString, + VIR_XML_PROP_NONZERO, + &def->data.emulator.version) < 0) goto error; - } if (!(def->data.emulator.source = virDomainChrSourceDefNew(xmlopt))) goto error; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3362042db5..bab667d026 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1436,15 +1436,15 @@ typedef enum { #define VIR_DOMAIN_TPM_DEFAULT_DEVICE "/dev/tpm0" struct _virDomainTPMDef { - int type; /* virDomainTPMBackendType */ + virDomainTPMModel model; + virDomainTPMBackendType type; virDomainDeviceInfo info; - int model; /* virDomainTPMModel */ union { struct { virDomainChrSourceDef *source; } passthrough; struct { - int version; /* virDomainTPMVersion */ + virDomainTPMVersion version; virDomainChrSourceDef *source; char *storagepath; char *logfile; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 262fffe5fe..93ea8748c7 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9840,7 +9840,7 @@ qemuBuildTPMCommandLine(virCommand *cmd, g_autoptr(qemuFDPass) passtpm = NULL; g_autoptr(qemuFDPass) passcancel = NULL; - switch ((virDomainTPMBackendType) tpm->type) { + switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: { VIR_AUTOCLOSE fdtpm = -1; VIR_AUTOCLOSE fdcancel = -1; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0343fd3597..09fc88e7fa 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11752,7 +11752,7 @@ qemuDomainDeviceBackendChardevForeachOne(virDomainDeviceDef *dev, return cb(dev, dev->data.rng->source.chardev, opaque); case VIR_DOMAIN_DEVICE_TPM: - switch ((virDomainTPMBackendType) dev->data.tpm->type) { + switch (dev->data.tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: return cb(dev, dev->data.tpm->data.passthrough.source, opaque); -- 2.35.1
On Mon, Jul 18, 2022 at 11:30:47 +0200, Michal Privoznik wrote:
When parsing a TPM device plenty of virXMLPropString() + enum2int() combos are used. These can be replaced with virXMLPropEnum().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 39 ++++++++++++--------------------------- src/conf/domain_conf.h | 6 +++--- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 2 +- 4 files changed, 17 insertions(+), 32 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 28f0e75e60..6263d90fdb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
[...]
@@ -10358,13 +10355,11 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt,
def = g_new0(virDomainTPMDef, 1);
- model = virXMLPropString(node, "model"); - if (model != NULL && - (def->model = virDomainTPMModelTypeFromString(model)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unknown TPM frontend model '%s'"), model); + if (virXMLPropEnum(node, "model", + virDomainTPMModelTypeFromString, + VIR_XML_PROP_NONZERO,
^^^
+ &def->model) < 0) goto error; - }
ctxt->node = node;
[...]
@@ -10407,14 +10395,11 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, def->data.passthrough.source->data.file.path = g_steal_pointer(&path); break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: - version = virXMLPropString(backends[0], "version"); - if (version && - (def->data.emulator.version = virDomainTPMVersionTypeFromString(version)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unsupported TPM version '%s'"), - version); + if (virXMLPropEnum(backends[0], "version", + virDomainTPMVersionTypeFromString, + VIR_XML_PROP_NONZERO,
^^^
+ &def->data.emulator.version) < 0) goto error; - }
if (!(def->data.emulator.source = virDomainChrSourceDefNew(xmlopt))) goto error;
Lines marked as '^^^' will likely need to be adapted per reasons in previous replies. Other than that: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
In the qemuDomainDefPostParse() we aim to fill in top level values, which require overall view of domain, or those parts of configuration that are not a device in domain XML (e.g. vCPUs). However, inside of qemuDomainDefTPMsPostParse(), which is called from aforementioned function, we do two tings: 1) fill in missing info (TPM version), and 2) validate TPM definition. Now, if 1) is moved into qemuDomainTPMDefPostParse() (the device post parse callback), then 2) can be moved into validation step. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 09fc88e7fa..bcee4d2602 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4593,17 +4593,6 @@ qemuDomainDefTPMsPostParse(virDomainDef *def) for (i = 0; i < def->ntpms; i++) { virDomainTPMDef *tpm = def->tpms[i]; - /* TPM 1.2 and 2 are not compatible, so we choose a specific version here */ - if (tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR && - tpm->data.emulator.version == VIR_DOMAIN_TPM_VERSION_DEFAULT) { - if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR || - tpm->model == VIR_DOMAIN_TPM_MODEL_CRB || - qemuDomainIsARMVirt(def)) - tpm->data.emulator.version = VIR_DOMAIN_TPM_VERSION_2_0; - else - tpm->data.emulator.version = VIR_DOMAIN_TPM_VERSION_1_2; - } - if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY) { if (proxyTPM) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -5806,15 +5795,26 @@ qemuDomainHostdevDefPostParse(virDomainHostdevDef *hostdev, static int qemuDomainTPMDefPostParse(virDomainTPMDef *tpm, - virArch arch) + const virDomainDef *def) { if (tpm->model == VIR_DOMAIN_TPM_MODEL_DEFAULT) { - if (ARCH_IS_PPC64(arch)) + if (ARCH_IS_PPC64(def->os.arch)) tpm->model = VIR_DOMAIN_TPM_MODEL_SPAPR; else tpm->model = VIR_DOMAIN_TPM_MODEL_TIS; } + /* TPM 1.2 and 2 are not compatible, so we choose a specific version here */ + if (tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR && + tpm->data.emulator.version == VIR_DOMAIN_TPM_VERSION_DEFAULT) { + if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR || + tpm->model == VIR_DOMAIN_TPM_MODEL_CRB || + qemuDomainIsARMVirt(def)) + tpm->data.emulator.version = VIR_DOMAIN_TPM_VERSION_2_0; + else + tpm->data.emulator.version = VIR_DOMAIN_TPM_VERSION_1_2; + } + return 0; } @@ -5941,7 +5941,7 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDef *dev, break; case VIR_DOMAIN_DEVICE_TPM: - ret = qemuDomainTPMDefPostParse(dev->data.tpm, def->os.arch); + ret = qemuDomainTPMDefPostParse(dev->data.tpm, def); break; case VIR_DOMAIN_DEVICE_MEMORY: -- 2.35.1
On Mon, Jul 18, 2022 at 11:30:48 +0200, Michal Privoznik wrote:
In the qemuDomainDefPostParse() we aim to fill in top level values, which require overall view of domain, or those parts of configuration that are not a device in domain XML (e.g. vCPUs). However, inside of qemuDomainDefTPMsPostParse(), which is called from aforementioned function, we do two tings:
1) fill in missing info (TPM version), and 2) validate TPM definition.
Now, if 1) is moved into qemuDomainTPMDefPostParse() (the device post parse callback), then 2) can be moved into validation step.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 28 ++++++++++++++--------------
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
After previous cleanup, the qemuDomainDefTPMsPostParse() function does nothing more than validates TPM devices. Therefore, it should live in qemu_validate.c instead of qemu_domain.c. Move it there and rename to reflect the fact that the function is doing validation instead of PostParsing. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 34 ---------------------------------- src/qemu/qemu_validate.c | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 34 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index bcee4d2602..e3d1bb548f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4583,37 +4583,6 @@ qemuDomainDefNumaCPUsPostParse(virDomainDef *def, } -static int -qemuDomainDefTPMsPostParse(virDomainDef *def) -{ - virDomainTPMDef *proxyTPM = NULL; - virDomainTPMDef *regularTPM = NULL; - size_t i; - - for (i = 0; i < def->ntpms; i++) { - virDomainTPMDef *tpm = def->tpms[i]; - - if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY) { - if (proxyTPM) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("only a single TPM Proxy device is supported")); - return -1; - } else { - proxyTPM = tpm; - } - } else if (regularTPM) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("only a single TPM non-proxy device is supported")); - return -1; - } else { - regularTPM = tpm; - } - } - - return 0; -} - - static int qemuDomainDefPostParseBasic(virDomainDef *def, void *opaque G_GNUC_UNUSED) @@ -4709,9 +4678,6 @@ qemuDomainDefPostParse(virDomainDef *def, if (qemuDomainDefNumaCPUsPostParse(def, qemuCaps) < 0) return -1; - if (qemuDomainDefTPMsPostParse(def) < 0) - return -1; - return 0; } diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index ff164118b7..ce8f92f301 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1113,6 +1113,37 @@ qemuValidateDomainDefPanic(const virDomainDef *def, } +static int +qemuValidateDomainDefTPMs(const virDomainDef *def) +{ + const virDomainTPMDef *proxyTPM = NULL; + const virDomainTPMDef *regularTPM = NULL; + size_t i; + + for (i = 0; i < def->ntpms; i++) { + virDomainTPMDef *tpm = def->tpms[i]; + + if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY) { + if (proxyTPM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only a single TPM Proxy device is supported")); + return -1; + } else { + proxyTPM = tpm; + } + } else if (regularTPM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only a single TPM non-proxy device is supported")); + return -1; + } else { + regularTPM = tpm; + } + } + + return 0; +} + + int qemuValidateLifecycleAction(virDomainLifecycleAction onPoweroff, virDomainLifecycleAction onReboot, @@ -1310,6 +1341,9 @@ qemuValidateDomainDef(const virDomainDef *def, if (qemuValidateDomainDefPanic(def, qemuCaps) < 0) return -1; + if (qemuValidateDomainDefTPMs(def) < 0) + return -1; + if (def->sec) { switch ((virDomainLaunchSecurity) def->sec->sectype) { case VIR_DOMAIN_LAUNCH_SECURITY_SEV: -- 2.35.1
On Mon, Jul 18, 2022 at 11:30:49 +0200, Michal Privoznik wrote:
After previous cleanup, the qemuDomainDefTPMsPostParse() function does nothing more than validates TPM devices. Therefore, it should live in qemu_validate.c instead of qemu_domain.c. Move it there and rename to reflect the fact that the function is doing validation instead of PostParsing.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 34 ---------------------------------- src/qemu/qemu_validate.c | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 34 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index bcee4d2602..e3d1bb548f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c
[...]
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index ff164118b7..ce8f92f301 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1113,6 +1113,37 @@ qemuValidateDomainDefPanic(const virDomainDef *def, }
+static int +qemuValidateDomainDefTPMs(const virDomainDef *def) +{ + const virDomainTPMDef *proxyTPM = NULL; + const virDomainTPMDef *regularTPM = NULL; + size_t i;
Please refactor the function to un-do unnecessarily complex logic:
+ + for (i = 0; i < def->ntpms; i++) { + virDomainTPMDef *tpm = def->tpms[i]; + + if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY) { + if (proxyTPM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only a single TPM Proxy device is supported")); + return -1; + } else { + proxyTPM = tpm; + }
This else section is not needed, and proxyTPM can be assigned unconditionally.
+ } else if (regularTPM) {
Make this into a plain 'else' and move this condition inside ...
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only a single TPM non-proxy device is supported")); + return -1; + } else { + regularTPM = tpm;
... and remove this else branch.
+ } + } + + return 0; +}
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
When no TPM version is provided in the input XML we may default to version 2.0 (see qemuDomainTPMDefPostParse()). However, <active_pcr_banks/> are parsed iff a version 2.0 was specified. This means that this piece of information might be lost. It's better to parse everything we've been given and then validate that the configuration is valid. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2084046 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 21 ++++++++++----------- src/conf/domain_validate.c | 28 +++++++++++++++++++++++++++- 2 files changed, 37 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6263d90fdb..610fa5262b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10422,18 +10422,17 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, goto error; } } - if (def->data.emulator.version == VIR_DOMAIN_TPM_VERSION_2_0) { - if ((nnodes = virXPathNodeSet("./backend/active_pcr_banks/*", ctxt, &nodes)) < 0) - break; - for (i = 0; i < nnodes; i++) { - if ((bank = virDomainTPMPcrBankTypeFromString((const char *)nodes[i]->name)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unsupported PCR banks '%s'"), - nodes[i]->name); - goto error; - } - def->data.emulator.activePcrBanks |= (1 << bank); + + if ((nnodes = virXPathNodeSet("./backend/active_pcr_banks/*", ctxt, &nodes)) < 0) + break; + for (i = 0; i < nnodes; i++) { + if ((bank = virDomainTPMPcrBankTypeFromString((const char *)nodes[i]->name)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported PCR banks '%s'"), + nodes[i]->name); + goto error; } + def->data.emulator.activePcrBanks |= (1 << bank); } break; case VIR_DOMAIN_TPM_TYPE_LAST: diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 814922cd46..8d4a69f127 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -2611,6 +2611,30 @@ virDomainIOMMUDefValidate(const virDomainIOMMUDef *iommu) return 0; } + +static int +virDomainTPMDevValidate(const virDomainTPMDef *tpm) +{ + switch (tpm->type) { + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + if (tpm->data.emulator.activePcrBanks && + tpm->data.emulator.version != VIR_DOMAIN_TPM_VERSION_2_0) { + virReportError(VIR_ERR_XML_ERROR, + _("<active_pcr_banks/> requires TPM version '%s'"), + virDomainTPMVersionTypeToString(VIR_DOMAIN_TPM_VERSION_2_0)); + return -1; + } + break; + + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: + case VIR_DOMAIN_TPM_TYPE_LAST: + break; + } + + return 0; +} + + static int virDomainDeviceInfoValidate(const virDomainDeviceDef *dev) { @@ -2715,12 +2739,14 @@ virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev, case VIR_DOMAIN_DEVICE_IOMMU: return virDomainIOMMUDefValidate(dev->data.iommu); + case VIR_DOMAIN_DEVICE_TPM: + return virDomainTPMDevValidate(dev->data.tpm); + case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_WATCHDOG: case VIR_DOMAIN_DEVICE_HUB: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: - case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LAST: -- 2.35.1
On Mon, Jul 18, 2022 at 11:30:50 +0200, Michal Privoznik wrote:
When no TPM version is provided in the input XML we may default to version 2.0 (see qemuDomainTPMDefPostParse()). However, <active_pcr_banks/> are parsed iff a version 2.0 was specified. This means that this piece of information might be lost.
It's better to parse everything we've been given and then validate that the configuration is valid.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2084046 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 21 ++++++++++----------- src/conf/domain_validate.c | 28 +++++++++++++++++++++++++++- 2 files changed, 37 insertions(+), 12 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6263d90fdb..610fa5262b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10422,18 +10422,17 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, goto error; } } - if (def->data.emulator.version == VIR_DOMAIN_TPM_VERSION_2_0) { - if ((nnodes = virXPathNodeSet("./backend/active_pcr_banks/*", ctxt, &nodes)) < 0) - break; - for (i = 0; i < nnodes; i++) { - if ((bank = virDomainTPMPcrBankTypeFromString((const char *)nodes[i]->name)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unsupported PCR banks '%s'"), - nodes[i]->name); - goto error; - } - def->data.emulator.activePcrBanks |= (1 << bank); + + if ((nnodes = virXPathNodeSet("./backend/active_pcr_banks/*", ctxt, &nodes)) < 0) + break; + for (i = 0; i < nnodes; i++) { + if ((bank = virDomainTPMPcrBankTypeFromString((const char *)nodes[i]->name)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported PCR banks '%s'"), + nodes[i]->name); + goto error; } + def->data.emulator.activePcrBanks |= (1 << bank);
Ewww. This is clearly a job for virBitmap. I'll post a patch to refactor it on top of this patch, so don't worry about it or conflicts. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On 7/18/22 11:30, Michal Privoznik wrote:
*** BLURB HERE ***
Michal Prívozník (8): conf: Report an error when default TPM model is provided conf: Report error when default TPM version is provided conf: Drop needless setting of VIR_DOMAIN_TPM_VERSION_DEFAULT conf: Move _virDomainTPMDef::version into _virDomainTPMDef::data::emulator conf: Use virXMLPropEnum more when parsing TPM qemu_domain: Move TPM post parse code into qemuDomainTPMDefPostParse() qemu: Move TPMs validation out of PostParse conf: Don't lose <active_pcr_banks/> when no TPM version is provided
src/conf/domain_conf.c | 81 +++++++++++++------------------- src/conf/domain_conf.h | 10 ++-- src/conf/domain_validate.c | 28 ++++++++++- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 63 ++++++------------------- src/qemu/qemu_tpm.c | 10 ++-- src/qemu/qemu_validate.c | 87 ++++++++++++++++++++++++----------- src/security/virt-aa-helper.c | 2 +- 8 files changed, 147 insertions(+), 136 deletions(-)
Polite ping. Michal
participants (3)
-
Michal Privoznik -
Michal Prívozník
-
Peter Krempa