If we do not have a persistent definition, there's no point in
looking for it since we cannot store it.

This fixes the crash when starting a transient domain.

https://issues.redhat.com/browse/RHEL-69774

Fixes: d79542eec669eb9c449bb8228179e7a87e768017
Signed-off-by: Ján Tomko <jtomko@redhat.com>
---
 src/qemu/qemu_extdevice.c | 5 ++++-
 src/qemu/qemu_tpm.c       | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
index a6f31f9773..d4b6e11e0b 100644
--- a/src/qemu/qemu_extdevice.c
+++ b/src/qemu/qemu_extdevice.c
@@ -190,7 +190,10 @@ qemuExtDevicesStart(virQEMUDriver *driver,
 
     for (i = 0; i < def->ntpms; i++) {
         virDomainTPMDef *tpm = def->tpms[i];
-        virDomainTPMDef *persistentTPMDef = persistentDef->tpms[i];
+        virDomainTPMDef *persistentTPMDef = NULL;
+
+        if (persistentDef)
+            persistentTPMDef = persistentDef->tpms[i];
 
         if (tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR &&
             qemuExtTPMStart(driver, vm, tpm, persistentTPMDef,
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index f223dcb9ae..f5e0184e54 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -773,7 +773,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm,
                                 incomingMigration) < 0)
         goto error;
 
-    if (run_setup && !incomingMigration &&
+    if (run_setup && !incomingMigration && persistentTPMDef &&
         qemuTPMEmulatorUpdateProfileName(&tpm->data.emulator, persistentTPMDef,
                                          cfg, saveDef) < 0)
         goto error;
-- 
2.47.0

On Tue, Dec 03, 2024 at 12:06:37 +0100, Ján Tomko wrote:
> If we do not have a persistent definition, there's no point in
> looking for it since we cannot store it.
> 
> This fixes the crash when starting a transient domain.
> 
> https://issues.redhat.com/browse/RHEL-69774
> 
> Fixes: d79542eec669eb9c449bb8228179e7a87e768017
> Signed-off-by: Ján Tomko <jtomko@redhat.com>
> ---
>  src/qemu/qemu_extdevice.c | 5 ++++-
>  src/qemu/qemu_tpm.c       | 2 +-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
> index a6f31f9773..d4b6e11e0b 100644
> --- a/src/qemu/qemu_extdevice.c
> +++ b/src/qemu/qemu_extdevice.c
> @@ -190,7 +190,10 @@ qemuExtDevicesStart(virQEMUDriver *driver,
>  
>      for (i = 0; i < def->ntpms; i++) {
>          virDomainTPMDef *tpm = def->tpms[i];
> -        virDomainTPMDef *persistentTPMDef = persistentDef->tpms[i];
> +        virDomainTPMDef *persistentTPMDef = NULL;
> +
> +        if (persistentDef)
> +            persistentTPMDef = persistentDef->tpms[i];

And what if the persistent definition has a different number of tpm
devices? We might be starting a domain using custom XML which is
completely different from the persistent definition.

And even if both active and persistent definition contains the same
number of tpm devices, would there be a problem if the devices
themselves did not match (if it can happen, I know mostly nothing about
tpm)?

Jirka

On Tue, Dec 03, 2024 at 12:33:50PM +0100, Jiri Denemark wrote:
> On Tue, Dec 03, 2024 at 12:06:37 +0100, Ján Tomko wrote:
> > If we do not have a persistent definition, there's no point in
> > looking for it since we cannot store it.
> > 
> > This fixes the crash when starting a transient domain.
> > 
> > https://issues.redhat.com/browse/RHEL-69774
> > 
> > Fixes: d79542eec669eb9c449bb8228179e7a87e768017
> > Signed-off-by: Ján Tomko <jtomko@redhat.com>
> > ---
> >  src/qemu/qemu_extdevice.c | 5 ++++-
> >  src/qemu/qemu_tpm.c       | 2 +-
> >  2 files changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
> > index a6f31f9773..d4b6e11e0b 100644
> > --- a/src/qemu/qemu_extdevice.c
> > +++ b/src/qemu/qemu_extdevice.c
> > @@ -190,7 +190,10 @@ qemuExtDevicesStart(virQEMUDriver *driver,
> >  
> >      for (i = 0; i < def->ntpms; i++) {
> >          virDomainTPMDef *tpm = def->tpms[i];
> > -        virDomainTPMDef *persistentTPMDef = persistentDef->tpms[i];
> > +        virDomainTPMDef *persistentTPMDef = NULL;
> > +
> > +        if (persistentDef)
> > +            persistentTPMDef = persistentDef->tpms[i];
> 
> And what if the persistent definition has a different number of tpm
> devices? We might be starting a domain using custom XML which is
> completely different from the persistent definition.
> 
> And even if both active and persistent definition contains the same
> number of tpm devices, would there be a problem if the devices
> themselves did not match (if it can happen, I know mostly nothing about
> tpm)?

We support a max of two TPM devices - validated during parsing.

Originally we only allowed 1, but 19d74fdf0eb5d2e89e80ceedea736425160ffccb
raised that to 2, saying it was valid to have a proxy device alongside an
emulated device, but it didn't validte that they 2 devices were indeed
different AFAICT :-(

So I guess we should validate that the TPM backend type matches before
doing this copy.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|