From 824e349397b1f1fa5d7c1c5d21c88e04df21bf17 Mon Sep 17 00:00:00 2001 From: Michal Privoznik Date: Sat, 27 Jun 2020 06:28:17 +0200 Subject: [PATCH] qemu: Use qemuSecuritySetSavedStateLabel() to label restore path Currently, when restoring from a domain the path that the domain restores from is labelled under qemuSecuritySetAllLabel() (and after v6.3.0-rc1~108 even outside transactions). While this grants QEMU the access, it has a flaw, because once the domain is restored, up and running then qemuSecurityDomainRestorePathLabel() is called, which is not real counterpart. In case of DAC driver the SetAllLabel() does nothing with the restore path but RestorePathLabel() does - it chown()-s the file back and since there is no original label remembered, the file is chown()-ed to root:root. While the apparent solution is to have DAC driver set the label (and thus remember the original one) in SetAllLabel(), we can do better. Turns out, we are opening the file ourselves (because it may live on a root squashed NFS) and then are just passing the FD to QEMU. But this means, that we don't have to chown() the file at all, we need to set SELinux labels and/or add the path to AppArmor profile. And since we want to restore labels right after QEMU is done loading the migration stream (we don't want to wait until qemuSecurityRestoreAllLabel()), the best way to approach this is to have separate APIs for labelling and restoring label on the restore file. I will investigate whether AppArmor can use the SavedStateLabel() API instead of passing the restore path to SetAllLabel(). Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1851016 Signed-off-by: Michal Privoznik Reviewed-by: Erik Skultety --- src/qemu/qemu_driver.c | 2 -- src/qemu/qemu_process.c | 12 ++++++++++++ src/qemu/qemu_security.c | 7 ------- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5ee5b9ffe6..8ea4197d00 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6952,8 +6952,6 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, asyncJob, VIR_QEMU_PROCESS_STOP_MIGRATED); } - if (qemuSecurityDomainRestorePathLabel(driver, vm, path, true) < 0) - VIR_WARN("failed to restore save state label on %s", path); return ret; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d36088ba98..70fc24b993 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7073,6 +7073,7 @@ qemuProcessStart(virConnectPtr conn, qemuProcessIncomingDefPtr incoming = NULL; unsigned int stopFlags; bool relabel = false; + bool relabelSavedState = false; int ret = -1; int rv; @@ -7109,6 +7110,13 @@ qemuProcessStart(virConnectPtr conn, if (qemuProcessPrepareHost(driver, vm, flags) < 0) goto stop; + if (migratePath) { + if (qemuSecuritySetSavedStateLabel(driver->securityManager, + vm->def, migratePath) < 0) + goto cleanup; + relabelSavedState = true; + } + if ((rv = qemuProcessLaunch(conn, driver, vm, asyncJob, incoming, snapshot, vmop, flags)) < 0) { if (rv == -2) @@ -7145,6 +7153,10 @@ qemuProcessStart(virConnectPtr conn, ret = 0; cleanup: + if (relabelSavedState && + qemuSecurityRestoreSavedStateLabel(driver->securityManager, + vm->def, migratePath) < 0) + VIR_WARN("failed to restore save state label on %s", migratePath); qemuProcessIncomingDefFree(incoming); return ret; diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index f93d189df9..88925be2ac 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -39,13 +39,6 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; - /* Explicitly run this outside of transaction. We really want to relabel - * the file in the host and not in the domain's namespace. */ - if (virSecurityManagerDomainSetPathLabelRO(driver->securityManager, - vm->def, - stdin_path) < 0) - goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) pid = vm->pid; -- 2.47.2