[libvirt] [PATCH] Revert "tpm: Check TPM XML device configuration changes after edit"

Ján Tomko posted 1 patch 17 weeks ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/618fa97a0bb0c193df72e39d13dfb4174b0f1a8b.1565345731.git.jtomko@redhat.com
src/conf/domain_conf.c    | 56 ---------------------------------------
src/conf/domain_conf.h    |  3 ---
src/libvirt_private.syms  |  1 -
src/qemu/qemu_driver.c    | 28 --------------------
src/qemu/qemu_extdevice.c |  2 +-
src/qemu/qemu_extdevice.h |  3 ---
6 files changed, 1 insertion(+), 92 deletions(-)

[libvirt] [PATCH] Revert "tpm: Check TPM XML device configuration changes after edit"

Posted by Ján Tomko 17 weeks ago
Redefining a domain via virDomainDefineXML should not give different results
based on an already existing definition.

Also, there's a crasher somewhere in the code:
https://bugzilla.redhat.com/show_bug.cgi?id=1739338

This reverts commit 94b3aa55f83ada33a9fdda66068d58ef1a56c0a5
---
 src/conf/domain_conf.c    | 56 ---------------------------------------
 src/conf/domain_conf.h    |  3 ---
 src/libvirt_private.syms  |  1 -
 src/qemu/qemu_driver.c    | 28 --------------------
 src/qemu/qemu_extdevice.c |  2 +-
 src/qemu/qemu_extdevice.h |  3 ---
 6 files changed, 1 insertion(+), 92 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0456369d55..f54acd0c2a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -31461,59 +31461,3 @@ virDomainGraphicsNeedsAutoRenderNode(const virDomainGraphicsDef *graphics)
 
     return true;
 }
-
-
-static int
-virDomainCheckTPMChanges(virDomainDefPtr def,
-                         virDomainDefPtr newDef)
-{
-    bool oldEnc, newEnc;
-
-    if (!def->tpm)
-        return 0;
-
-    switch (def->tpm->type) {
-    case VIR_DOMAIN_TPM_TYPE_EMULATOR:
-        if (virFileExists(def->tpm->data.emulator.storagepath)) {
-            /* VM has been started */
-            /* Once a VM was started with an encrypted state we allow
-             * less configuration changes.
-             */
-            oldEnc = def->tpm->data.emulator.hassecretuuid;
-            if (oldEnc && def->tpm->type != newDef->tpm->type) {
-                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                               _("Changing the type of TPM is not allowed"));
-                return -1;
-            }
-            if (oldEnc && !newDef->tpm) {
-                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                               _("Removing an encrypted TPM is not allowed"));
-                return -1;
-            }
-            newEnc = newDef->tpm->data.emulator.hassecretuuid;
-            if (oldEnc != newEnc) {
-                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                   _("TPM state encryption cannot be changed "
-                     "once VM was started"));
-                return -1;
-            }
-        }
-        break;
-    case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
-    case VIR_DOMAIN_TPM_TYPE_LAST:
-        break;
-    }
-
-    return 0;
-}
-
-
-int
-virDomainCheckDeviceChanges(virDomainDefPtr def,
-                            virDomainDefPtr newDef)
-{
-    if (!def || !newDef)
-        return 0;
-
-    return virDomainCheckTPMChanges(def, newDef);
-}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 57ca2a8ad1..2d7350e675 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3641,6 +3641,3 @@ virDomainGraphicsGetRenderNode(const virDomainGraphicsDef *graphics);
 
 bool
 virDomainGraphicsNeedsAutoRenderNode(const virDomainGraphicsDef *graphics);
-
-int
-virDomainCheckDeviceChanges(virDomainDefPtr def, virDomainDefPtr newDef);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index cae8febf8d..7a3feb8efa 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -217,7 +217,6 @@ virDomainBootTypeFromString;
 virDomainBootTypeToString;
 virDomainCapabilitiesPolicyTypeToString;
 virDomainCapsFeatureTypeToString;
-virDomainCheckDeviceChanges;
 virDomainChrConsoleTargetTypeFromString;
 virDomainChrConsoleTargetTypeToString;
 virDomainChrDefForeach;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ff83d1c024..0fccf3dc37 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -53,7 +53,6 @@
 #include "qemu_migration_params.h"
 #include "qemu_blockjob.h"
 #include "qemu_security.h"
-#include "qemu_extdevice.h"
 
 #include "virerror.h"
 #include "virlog.h"
@@ -7760,30 +7759,6 @@ qemuDomainCreate(virDomainPtr dom)
     return qemuDomainCreateWithFlags(dom, 0);
 }
 
-static int
-qemuDomainCheckDeviceChanges(virQEMUDriverPtr driver,
-                             virDomainDefPtr def)
-{
-    virDomainObjPtr vm;
-    int ret;
-
-    vm = virDomainObjListFindByUUID(driver->domains, def->uuid);
-    if (!vm)
-        return 0;
-
-    if (qemuExtDevicesInitPaths(driver, vm->def) < 0) {
-        ret = -1;
-        goto cleanup;
-    }
-
-    ret = virDomainCheckDeviceChanges(vm->def, def);
-
- cleanup:
-    virDomainObjEndAPI(&vm);
-
-    return ret;
-}
-
 static virDomainPtr
 qemuDomainDefineXMLFlags(virConnectPtr conn,
                          const char *xml,
@@ -7820,9 +7795,6 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
     if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0)
         goto cleanup;
 
-    if (qemuDomainCheckDeviceChanges(driver, def) < 0)
-        goto cleanup;
-
     if (!(vm = virDomainObjListAdd(driver->domains, def,
                                    driver->xmlopt,
                                    0, &oldDef)))
diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
index af52466421..dc032aa60c 100644
--- a/src/qemu/qemu_extdevice.c
+++ b/src/qemu/qemu_extdevice.c
@@ -79,7 +79,7 @@ qemuExtDeviceLogCommand(qemuDomainLogContextPtr logCtxt,
  * stored and we can remove directories and files in case of domain XML
  * changes.
  */
-int
+static int
 qemuExtDevicesInitPaths(virQEMUDriverPtr driver,
                         virDomainDefPtr def)
 {
diff --git a/src/qemu/qemu_extdevice.h b/src/qemu/qemu_extdevice.h
index 5a53c79f38..039b3e60dd 100644
--- a/src/qemu/qemu_extdevice.h
+++ b/src/qemu/qemu_extdevice.h
@@ -54,6 +54,3 @@ bool qemuExtDevicesHasDevice(virDomainDefPtr def);
 int qemuExtDevicesSetupCgroup(virQEMUDriverPtr driver,
                               virDomainDefPtr def,
                               virCgroupPtr cgroup);
-
-int qemuExtDevicesInitPaths(virQEMUDriverPtr driver,
-                            virDomainDefPtr def);
-- 
2.19.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Revert "tpm: Check TPM XML device configuration changes after edit"

Posted by Stefan Berger 17 weeks ago
On 8/9/19 6:15 AM, Ján Tomko wrote:
> Redefining a domain via virDomainDefineXML should not give different results
> based on an already existing definition.


I added this patch so that users don't try to change a VM from encrypted 
to unencrypted on the level of the domain XML and assume it will start. 
It will not start anymore.


    Stefan

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Revert "tpm: Check TPM XML device configuration changes after edit"

Posted by Peter Krempa 17 weeks ago
On Mon, Aug 12, 2019 at 13:57:40 -0400, Stefan Berger wrote:
> On 8/9/19 6:15 AM, Ján Tomko wrote:
> > Redefining a domain via virDomainDefineXML should not give different results
> > based on an already existing definition.
> 
> 
> I added this patch so that users don't try to change a VM from encrypted to
> unencrypted on the level of the domain XML and assume it will start. It will
> not start anymore.

It is pointless to even try to protect from this as user can undefine
the domain and then define it again. Since at that point there's nothing
to compare against you'd get into the same situation.

The define API must have the same semantics when a new VM is defined and
when an pre-existing object is modified (well except let's say the UUID)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Revert "tpm: Check TPM XML device configuration changes after edit"

Posted by Stefan Berger 17 weeks ago
On 8/12/19 2:11 PM, Peter Krempa wrote:
> On Mon, Aug 12, 2019 at 13:57:40 -0400, Stefan Berger wrote:
>> On 8/9/19 6:15 AM, Ján Tomko wrote:
>>> Redefining a domain via virDomainDefineXML should not give different results
>>> based on an already existing definition.
>>
>> I added this patch so that users don't try to change a VM from encrypted to
>> unencrypted on the level of the domain XML and assume it will start. It will
>> not start anymore.
> It is pointless to even try to protect from this as user can undefine
> the domain and then define it again. Since at that point there's nothing
> to compare against you'd get into the same situation.
Not quite. We would deleted the state directories of the (encrypted) 
TPMs upon VM undefinition. So the user would start with no existing TPM 
state.
>
> The define API must have the same semantics when a new VM is defined and
> when an pre-existing object is modified (well except let's say the UUID)


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Revert "tpm: Check TPM XML device configuration changes after edit"

Posted by Peter Krempa 17 weeks ago
On Mon, Aug 12, 2019 at 14:15:22 -0400, Stefan Berger wrote:
> On 8/12/19 2:11 PM, Peter Krempa wrote:
> > On Mon, Aug 12, 2019 at 13:57:40 -0400, Stefan Berger wrote:
> > > On 8/9/19 6:15 AM, Ján Tomko wrote:
> > > > Redefining a domain via virDomainDefineXML should not give different results
> > > > based on an already existing definition.
> > > 
> > > I added this patch so that users don't try to change a VM from encrypted to
> > > unencrypted on the level of the domain XML and assume it will start. It will
> > > not start anymore.
> > It is pointless to even try to protect from this as user can undefine
> > the domain and then define it again. Since at that point there's nothing
> > to compare against you'd get into the same situation.
> Not quite. We would deleted the state directories of the (encrypted) TPMs
> upon VM undefinition. So the user would start with no existing TPM state.

Well, that is okay. If the user changes configuration such as that the
wrong state is reused it's a configuration problem. It's the same
situation as if the user changes from UEFI to bios but provides the
wrong/old path to the bios image.

We just must make sure that once the original configuration is restored
(including the disk images (if the guest would e.g. record that the
state has changed and refuse to work)) everything works as if nothing
happened. This is the snapshot revert scenario (but this requires some
management level trickery to e.g. also revert the state of the TPM (as I
doubt that the code is more clever than in the case of the UEFI variable
store which we don't snapshot currently properly))
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Revert "tpm: Check TPM XML device configuration changes after edit"

Posted by Marc-André Lureau 17 weeks ago
Hi

On Fri, Aug 9, 2019 at 2:15 PM Ján Tomko <jtomko@redhat.com> wrote:
>
> Redefining a domain via virDomainDefineXML should not give different results
> based on an already existing definition.
>
> Also, there's a crasher somewhere in the code:
> https://bugzilla.redhat.com/show_bug.cgi?id=1739338
>
> This reverts commit 94b3aa55f83ada33a9fdda66068d58ef1a56c0a5
> ---
>  src/conf/domain_conf.c    | 56 ---------------------------------------
>  src/conf/domain_conf.h    |  3 ---
>  src/libvirt_private.syms  |  1 -
>  src/qemu/qemu_driver.c    | 28 --------------------
>  src/qemu/qemu_extdevice.c |  2 +-
>  src/qemu/qemu_extdevice.h |  3 ---
>  6 files changed, 1 insertion(+), 92 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 0456369d55..f54acd0c2a 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -31461,59 +31461,3 @@ virDomainGraphicsNeedsAutoRenderNode(const virDomainGraphicsDef *graphics)
>
>      return true;
>  }
> -
> -
> -static int
> -virDomainCheckTPMChanges(virDomainDefPtr def,
> -                         virDomainDefPtr newDef)
> -{
> -    bool oldEnc, newEnc;
> -
> -    if (!def->tpm)
> -        return 0;
> -
> -    switch (def->tpm->type) {
> -    case VIR_DOMAIN_TPM_TYPE_EMULATOR:
> -        if (virFileExists(def->tpm->data.emulator.storagepath)) {
> -            /* VM has been started */
> -            /* Once a VM was started with an encrypted state we allow
> -             * less configuration changes.
> -             */
> -            oldEnc = def->tpm->data.emulator.hassecretuuid;
> -            if (oldEnc && def->tpm->type != newDef->tpm->type) {
> -                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                               _("Changing the type of TPM is not allowed"));
> -                return -1;
> -            }
> -            if (oldEnc && !newDef->tpm) {
> -                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                               _("Removing an encrypted TPM is not allowed"));
> -                return -1;
> -            }
> -            newEnc = newDef->tpm->data.emulator.hassecretuuid;

The crash was there fwiw, neewdef->tpm is null.

> -            if (oldEnc != newEnc) {
> -                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                   _("TPM state encryption cannot be changed "
> -                     "once VM was started"));
> -                return -1;
> -            }
> -        }
> -        break;
> -    case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
> -    case VIR_DOMAIN_TPM_TYPE_LAST:
> -        break;
> -    }
> -
> -    return 0;
> -}
> -
> -
> -int
> -virDomainCheckDeviceChanges(virDomainDefPtr def,
> -                            virDomainDefPtr newDef)
> -{
> -    if (!def || !newDef)
> -        return 0;
> -
> -    return virDomainCheckTPMChanges(def, newDef);
> -}
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 57ca2a8ad1..2d7350e675 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -3641,6 +3641,3 @@ virDomainGraphicsGetRenderNode(const virDomainGraphicsDef *graphics);
>
>  bool
>  virDomainGraphicsNeedsAutoRenderNode(const virDomainGraphicsDef *graphics);
> -
> -int
> -virDomainCheckDeviceChanges(virDomainDefPtr def, virDomainDefPtr newDef);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index cae8febf8d..7a3feb8efa 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -217,7 +217,6 @@ virDomainBootTypeFromString;
>  virDomainBootTypeToString;
>  virDomainCapabilitiesPolicyTypeToString;
>  virDomainCapsFeatureTypeToString;
> -virDomainCheckDeviceChanges;
>  virDomainChrConsoleTargetTypeFromString;
>  virDomainChrConsoleTargetTypeToString;
>  virDomainChrDefForeach;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index ff83d1c024..0fccf3dc37 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -53,7 +53,6 @@
>  #include "qemu_migration_params.h"
>  #include "qemu_blockjob.h"
>  #include "qemu_security.h"
> -#include "qemu_extdevice.h"
>
>  #include "virerror.h"
>  #include "virlog.h"
> @@ -7760,30 +7759,6 @@ qemuDomainCreate(virDomainPtr dom)
>      return qemuDomainCreateWithFlags(dom, 0);
>  }
>
> -static int
> -qemuDomainCheckDeviceChanges(virQEMUDriverPtr driver,
> -                             virDomainDefPtr def)
> -{
> -    virDomainObjPtr vm;
> -    int ret;
> -
> -    vm = virDomainObjListFindByUUID(driver->domains, def->uuid);
> -    if (!vm)
> -        return 0;
> -
> -    if (qemuExtDevicesInitPaths(driver, vm->def) < 0) {
> -        ret = -1;
> -        goto cleanup;
> -    }
> -
> -    ret = virDomainCheckDeviceChanges(vm->def, def);
> -
> - cleanup:
> -    virDomainObjEndAPI(&vm);
> -
> -    return ret;
> -}
> -
>  static virDomainPtr
>  qemuDomainDefineXMLFlags(virConnectPtr conn,
>                           const char *xml,
> @@ -7820,9 +7795,6 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
>      if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0)
>          goto cleanup;
>
> -    if (qemuDomainCheckDeviceChanges(driver, def) < 0)
> -        goto cleanup;
> -
>      if (!(vm = virDomainObjListAdd(driver->domains, def,
>                                     driver->xmlopt,
>                                     0, &oldDef)))
> diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
> index af52466421..dc032aa60c 100644
> --- a/src/qemu/qemu_extdevice.c
> +++ b/src/qemu/qemu_extdevice.c
> @@ -79,7 +79,7 @@ qemuExtDeviceLogCommand(qemuDomainLogContextPtr logCtxt,
>   * stored and we can remove directories and files in case of domain XML
>   * changes.
>   */
> -int
> +static int
>  qemuExtDevicesInitPaths(virQEMUDriverPtr driver,
>                          virDomainDefPtr def)
>  {
> diff --git a/src/qemu/qemu_extdevice.h b/src/qemu/qemu_extdevice.h
> index 5a53c79f38..039b3e60dd 100644
> --- a/src/qemu/qemu_extdevice.h
> +++ b/src/qemu/qemu_extdevice.h
> @@ -54,6 +54,3 @@ bool qemuExtDevicesHasDevice(virDomainDefPtr def);
>  int qemuExtDevicesSetupCgroup(virQEMUDriverPtr driver,
>                                virDomainDefPtr def,
>                                virCgroupPtr cgroup);
> -
> -int qemuExtDevicesInitPaths(virQEMUDriverPtr driver,
> -                            virDomainDefPtr def);
> --
> 2.19.2
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list



-- 
Marc-André Lureau

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Revert "tpm: Check TPM XML device configuration changes after edit"

Posted by Jiri Denemark 17 weeks ago
On Fri, Aug 09, 2019 at 12:15:43 +0200, Ján Tomko wrote:
> Redefining a domain via virDomainDefineXML should not give different results
> based on an already existing definition.
> 
> Also, there's a crasher somewhere in the code:
> https://bugzilla.redhat.com/show_bug.cgi?id=1739338
> 
> This reverts commit 94b3aa55f83ada33a9fdda66068d58ef1a56c0a5

Right, this kind of check is only suitable for cold(un)plug.
Redefinition of the domain is only constrained by the new XML being
correct.

Reviewed-by: Jiri Denemark <jdenemar@redhat.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list