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

[Peter Krempa]

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.

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.