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

[Michal Prívozník]

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.

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