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@xxxxxxxxxx>
> >>>> ---
> >>>> 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@xxxxxxxxxx>
