[libvirt] [PATCH 0/2] Taint domains altered by hook scripts
Currently, we only allow XML changing in qemu driver and nowhere else. Michal Privoznik (2): virDomainTaintFlags: Introduce VIR_DOMAIN_TAINT_HOOK qemu: Implement VIR_DOMAIN_TAINT_HOOK src/conf/domain_conf.c | 3 ++- src/conf/domain_conf.h | 1 + src/qemu/qemu_domain.c | 4 ++++ src/qemu/qemu_domain.h | 2 ++ src/qemu/qemu_migration.c | 10 ++++++++++ 5 files changed, 19 insertions(+), 1 deletion(-) -- 1.8.5.2
This new flag is to be used for tainting domains which XML definition was altered at runtime by a hook script. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 3 ++- src/conf/domain_conf.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 28e24f9..98ac8c8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -107,7 +107,8 @@ VIR_ENUM_IMPL(virDomainTaint, VIR_DOMAIN_TAINT_LAST, "shell-scripts", "disk-probing", "external-launch", - "host-cpu"); + "host-cpu", + "hook-script"); VIR_ENUM_IMPL(virDomainVirt, VIR_DOMAIN_VIRT_LAST, "qemu", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index d8f2e49..dc5f8a1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2109,6 +2109,7 @@ enum virDomainTaintFlags { VIR_DOMAIN_TAINT_DISK_PROBING, /* Relying on potentially unsafe disk format probing */ VIR_DOMAIN_TAINT_EXTERNAL_LAUNCH, /* Externally launched guest domain */ VIR_DOMAIN_TAINT_HOST_CPU, /* Host CPU passthrough in use */ + VIR_DOMAIN_TAINT_HOOK, /* Domain (possibly) changed via hook script */ VIR_DOMAIN_TAINT_LAST }; -- 1.8.5.2
On 02/04/2014 05:49 PM, Michal Privoznik wrote:
This new flag is to be used for tainting domains which XML definition was altered at runtime by a hook script.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 3 ++- src/conf/domain_conf.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 28e24f9..98ac8c8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -107,7 +107,8 @@ VIR_ENUM_IMPL(virDomainTaint, VIR_DOMAIN_TAINT_LAST, "shell-scripts", "disk-probing", "external-launch", - "host-cpu"); + "host-cpu", + "hook-script");
So I came back to this series after considering network tainting again. In the case of networks, your patch just always tainted the network whenever a hook script was present. But in the case of domains, you're only tainting it if the hook script modified the XML *and* libvirt accepted/used that modified XML. This makes me think two things: 1) we should probably be consistent, so if we only taint the domain if the hook modifies the XML and we use that XML, then maybe we shouldn't taint networks just because a hook script was called (or maybe domains should always get a "hook-script" taint if a script is run at all, and a different taint if the hook modifies the XML - see (2)) 2) The real reason we're tainting the domain here is because a hook modified the xml, NOT just because a hook was run, so the reason should probably be something like "hook-modified-xml". In the future, we may want to also taint all domains that had a script run at all, and in that case we would still have "hook-script" available to use. Other than that, this and PATCH 2/2 are fine - ACK.
VIR_ENUM_IMPL(virDomainVirt, VIR_DOMAIN_VIRT_LAST, "qemu", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index d8f2e49..dc5f8a1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2109,6 +2109,7 @@ enum virDomainTaintFlags { VIR_DOMAIN_TAINT_DISK_PROBING, /* Relying on potentially unsafe disk format probing */ VIR_DOMAIN_TAINT_EXTERNAL_LAUNCH, /* Externally launched guest domain */ VIR_DOMAIN_TAINT_HOST_CPU, /* Host CPU passthrough in use */ + VIR_DOMAIN_TAINT_HOOK, /* Domain (possibly) changed via hook script */
VIR_DOMAIN_TAINT_LAST };
On 13.02.2014 12:40, Laine Stump wrote:
On 02/04/2014 05:49 PM, Michal Privoznik wrote:
This new flag is to be used for tainting domains which XML definition was altered at runtime by a hook script.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 3 ++- src/conf/domain_conf.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 28e24f9..98ac8c8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -107,7 +107,8 @@ VIR_ENUM_IMPL(virDomainTaint, VIR_DOMAIN_TAINT_LAST, "shell-scripts", "disk-probing", "external-launch", - "host-cpu"); + "host-cpu", + "hook-script");
So I came back to this series after considering network tainting again. In the case of networks, your patch just always tainted the network whenever a hook script was present. But in the case of domains, you're only tainting it if the hook script modified the XML *and* libvirt accepted/used that modified XML.
This makes me think two things:
1) we should probably be consistent, so if we only taint the domain if the hook modifies the XML and we use that XML, then maybe we shouldn't taint networks just because a hook script was called (or maybe domains should always get a "hook-script" taint if a script is run at all, and a different taint if the hook modifies the XML - see (2))
2) The real reason we're tainting the domain here is because a hook modified the xml, NOT just because a hook was run, so the reason should probably be something like "hook-modified-xml". In the future, we may want to also taint all domains that had a script run at all, and in that case we would still have "hook-script" available to use.
Yes, I'm aware of this difference. The reason I chose to implement it because in domain case hook scripts can't cause hypervisor malfunction, they merely adjust environment that hypervisor runs in. However, in network case this environment may cause losing connectivity. That's why I think hook scripts are more dangerous in then network case than in domain case. But maybe I'm wrong and we should be tainting domain whenever a hook script is run, regardless of its actual affect on the domain. I'll not push this one, until we have a resolution. Michal
On 13.02.2014 13:53, Michal Privoznik wrote:
On 13.02.2014 12:40, Laine Stump wrote:
On 02/04/2014 05:49 PM, Michal Privoznik wrote:
This new flag is to be used for tainting domains which XML definition was altered at runtime by a hook script.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 3 ++- src/conf/domain_conf.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 28e24f9..98ac8c8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -107,7 +107,8 @@ VIR_ENUM_IMPL(virDomainTaint, VIR_DOMAIN_TAINT_LAST, "shell-scripts", "disk-probing", "external-launch", - "host-cpu"); + "host-cpu", + "hook-script");
So I came back to this series after considering network tainting again. In the case of networks, your patch just always tainted the network whenever a hook script was present. But in the case of domains, you're only tainting it if the hook script modified the XML *and* libvirt accepted/used that modified XML.
This makes me think two things:
1) we should probably be consistent, so if we only taint the domain if the hook modifies the XML and we use that XML, then maybe we shouldn't taint networks just because a hook script was called (or maybe domains should always get a "hook-script" taint if a script is run at all, and a different taint if the hook modifies the XML - see (2))
2) The real reason we're tainting the domain here is because a hook modified the xml, NOT just because a hook was run, so the reason should probably be something like "hook-modified-xml". In the future, we may want to also taint all domains that had a script run at all, and in that case we would still have "hook-script" available to use.
Yes, I'm aware of this difference. The reason I chose to implement it because in domain case hook scripts can't cause hypervisor malfunction, they merely adjust environment that hypervisor runs in. However, in network case this environment may cause losing connectivity. That's why I think hook scripts are more dangerous in then network case than in domain case. But maybe I'm wrong and we should be tainting domain whenever a hook script is run, regardless of its actual affect on the domain.
I'll not push this one, until we have a resolution.
I saw DV's plan for freeze and pushed this. We can extend tainting to other cases anytime. But pushing new features is limited to development phase. So I've just pushed this even though I said I won't. There hasn't been much discussion anyway. Michal
Currently, there's just one place where we care if hook script is changing the domain XML: migration hook for incoming migration. In all other places where a hook script is executed, we don't read the XML back from the script. Anyway, the hook script can alter domain XML and hence we should taint it if the script did. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Notes: Do we want to mark all the cases where hook script was executed (but domain XML hasn't changed)? For instance, at domain startup process, the hook script is called and if it has exited with zero status, the startup process can continue, otherwise it's aborted. I don't think that counts as taint reason, does it? src/qemu/qemu_domain.c | 4 ++++ src/qemu/qemu_domain.h | 2 ++ src/qemu/qemu_migration.c | 10 ++++++++++ 3 files changed, 16 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c947e2e..3069462 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1628,6 +1628,7 @@ void qemuDomainObjCheckTaint(virQEMUDriverPtr driver, { size_t i; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + qemuDomainObjPrivatePtr priv = obj->privateData; if (cfg->privileged && (!cfg->clearEmulatorCapabilities || @@ -1635,6 +1636,9 @@ void qemuDomainObjCheckTaint(virQEMUDriverPtr driver, cfg->group == 0)) qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_HIGH_PRIVILEGES, logFD); + if (priv->hookRun) + qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_HOOK, logFD); + if (obj->def->namespaceData) { qemuDomainCmdlineDefPtr qemucmd = obj->def->namespaceData; if (qemucmd->num_args || qemucmd->num_env) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 6a92351..76a587f 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -173,6 +173,8 @@ struct _qemuDomainObjPrivate { virCond unplugFinished; /* signals that unpluggingDevice was unplugged */ const char *unpluggingDevice; /* alias of the device that is being unplugged */ char **qemuDevices; /* NULL-terminated list of devices aliases known to QEMU */ + + bool hookRun; /* true if there was a hook run over this domain */ }; typedef enum { diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 407fb70..664602c 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2181,6 +2181,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, virCapsPtr caps = NULL; char *migrateFrom = NULL; bool abort_on_error = !!(flags & VIR_MIGRATE_ABORT_ON_ERROR); + bool taint_hook = false; if (virTimeMillisNow(&now) < 0) return -1; @@ -2251,6 +2252,10 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, virDomainDefFree(*def); *def = newdef; + /* We should taint the domain here. However, @vm and therefore + * privateData too are still NULL, so just notice the fact and + * taint it later. */ + taint_hook = true; } } } @@ -2336,6 +2341,11 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, if (VIR_STRDUP(priv->origname, origname) < 0) goto cleanup; + if (taint_hook) { + /* Domain XML has been altered by a hook script. */ + priv->hookRun = true; + } + if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen, QEMU_MIGRATION_COOKIE_LOCKSTATE | QEMU_MIGRATION_COOKIE_NBD))) -- 1.8.5.2
On 04.02.2014 16:49, Michal Privoznik wrote:
Currently, we only allow XML changing in qemu driver and nowhere else.
Michal Privoznik (2): virDomainTaintFlags: Introduce VIR_DOMAIN_TAINT_HOOK qemu: Implement VIR_DOMAIN_TAINT_HOOK
src/conf/domain_conf.c | 3 ++- src/conf/domain_conf.h | 1 + src/qemu/qemu_domain.c | 4 ++++ src/qemu/qemu_domain.h | 2 ++ src/qemu/qemu_migration.c | 10 ++++++++++ 5 files changed, 19 insertions(+), 1 deletion(-)
Ping?
participants (2)
-
Laine Stump -
Michal Privoznik