[PATCH 1/8] conf: Report an error when default TPM model is provided

[Peter Krempa]

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 at 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 at redhat.com>