[PATCH 0/2] qemu: Move 2 pid files to stateDir
Please see the commit messages for detail. Peng Liang (2): qemu: Move pid file of pr-helper to stateDir qemu: Move pid file of virtiofsd to stateDir src/qemu/qemu_process.c | 6 ++++-- src/qemu/qemu_virtiofs.c | 8 ++++---- 2 files changed, 8 insertions(+), 6 deletions(-) -- 2.31.1
Libvirt will put the pid file of pr-helper to per-domain directory. However, the ownership of the per-domain directory is the user to run the QEMU process and the user has the write permission of the directory. If VM escape occurs, the attacker can 1. write arbitrary content to the pid file (if running QEMU using root), then the attacker can kill any process by writing appropriate pid to the pid file; 2. spoof the pid file (if running QEMU using a regular user), then the pr-helper process will never be cleared even if the VM is destroyed. So, move the pid file of pr-helper from per-domain directory to stateDir just like the pid file of the domain. Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/qemu/qemu_process.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1d0165af6daa..583f3ec76c7b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2859,9 +2859,11 @@ static char * qemuProcessBuildPRHelperPidfilePath(virDomainObj *vm) { qemuDomainObjPrivate *priv = vm->privateData; - const char *prdAlias = qemuDomainGetManagedPRAlias(); + g_autofree char *domname = virDomainDefGetShortName(vm->def); + g_autofree char *prdName = g_strdup_printf("%s-%s", domname, qemuDomainGetManagedPRAlias()); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); - return virPidFileBuildPath(priv->libDir, prdAlias); + return virPidFileBuildPath(cfg->stateDir, prdName); } -- 2.31.1
On 10/11/21 2:11 PM, Peng Liang wrote:
Libvirt will put the pid file of pr-helper to per-domain directory. However, the ownership of the per-domain directory is the user to run the QEMU process and the user has the write permission of the directory. If VM escape occurs, the attacker can 1. write arbitrary content to the pid file (if running QEMU using root), then the attacker can kill any process by writing appropriate pid to the pid file; 2. spoof the pid file (if running QEMU using a regular user), then the pr-helper process will never be cleared even if the VM is destroyed.
So, move the pid file of pr-helper from per-domain directory to stateDir just like the pid file of the domain.
Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/qemu/qemu_process.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1d0165af6daa..583f3ec76c7b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2859,9 +2859,11 @@ static char * qemuProcessBuildPRHelperPidfilePath(virDomainObj *vm) { qemuDomainObjPrivate *priv = vm->privateData; - const char *prdAlias = qemuDomainGetManagedPRAlias(); + g_autofree char *domname = virDomainDefGetShortName(vm->def); + g_autofree char *prdName = g_strdup_printf("%s-%s", domname, qemuDomainGetManagedPRAlias()); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver);
- return virPidFileBuildPath(priv->libDir, prdAlias); + return virPidFileBuildPath(cfg->stateDir, prdName); }
I don't think it is this simple. The pidfile path is not stored in status XML but generated when starting/stopping PR helper process. Therefore, if there is PR helper that was started with older libvirt its pidfile is /var/lib/libvirt/qemu/domain-1-fedora/pr-helper0.pid but after your change a different path is generated /var/run/libvirt/qemu/1-fedora-pr-helper0.pid and thus qemuProcessKillManagedPRDaemon() does not kill the PR helper process and leaves it behind. One solution would be to start recording the pidfile path in status XML and then when no path is found during parse we know that the old path was used. Michal
On 10/14/2021 6:10 PM, Michal Prívozník wrote:
On 10/11/21 2:11 PM, Peng Liang wrote:
Libvirt will put the pid file of pr-helper to per-domain directory. However, the ownership of the per-domain directory is the user to run the QEMU process and the user has the write permission of the directory. If VM escape occurs, the attacker can 1. write arbitrary content to the pid file (if running QEMU using root), then the attacker can kill any process by writing appropriate pid to the pid file; 2. spoof the pid file (if running QEMU using a regular user), then the pr-helper process will never be cleared even if the VM is destroyed.
So, move the pid file of pr-helper from per-domain directory to stateDir just like the pid file of the domain.
Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/qemu/qemu_process.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1d0165af6daa..583f3ec76c7b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2859,9 +2859,11 @@ static char * qemuProcessBuildPRHelperPidfilePath(virDomainObj *vm) { qemuDomainObjPrivate *priv = vm->privateData; - const char *prdAlias = qemuDomainGetManagedPRAlias(); + g_autofree char *domname = virDomainDefGetShortName(vm->def); + g_autofree char *prdName = g_strdup_printf("%s-%s", domname, qemuDomainGetManagedPRAlias()); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver);
- return virPidFileBuildPath(priv->libDir, prdAlias); + return virPidFileBuildPath(cfg->stateDir, prdName); }
I don't think it is this simple. The pidfile path is not stored in status XML but generated when starting/stopping PR helper process. Therefore, if there is PR helper that was started with older libvirt its pidfile is /var/lib/libvirt/qemu/domain-1-fedora/pr-helper0.pid but after your change a different path is generated /var/run/libvirt/qemu/1-fedora-pr-helper0.pid and thus qemuProcessKillManagedPRDaemon() does not kill the PR helper process and leaves it behind.
One solution would be to start recording the pidfile path in status XML and then when no path is found during parse we know that the old path was used.
Can we just change to use old path if the path returned by qemuProcessBuildPRHelperPidfilePath does not exist? For example: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f95ed80fac43..0ee1a97f5571 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2877,6 +2877,14 @@ qemuProcessKillManagedPRDaemon(virDomainObj *vm) return; } + if (!virFileExists(pidfile)) { + g_free(pidfile); + if (!(pidfile = qemuProcessBuildPRHelperPidfilePathOld(vm))) { + VIR_WARN("Unable to construct pr-helper pidfile path"); + return; + } + } + virErrorPreserveLast(&orig_err); if (virPidFileForceCleanupPath(pidfile) < 0) { VIR_WARN("Unable to kill pr-helper process"); qemuProcessBuildPRHelperPidfilePathOld will build the old path (e.g., /var/lib/libvirt/qemu/domain-1-fedora/pr-helper0.pid). Thanks, Peng
Michal
.
On 10/14/21 12:51 PM, Peng Liang wrote:
On 10/14/2021 6:10 PM, Michal Prívozník wrote:
On 10/11/21 2:11 PM, Peng Liang wrote:
Libvirt will put the pid file of pr-helper to per-domain directory. However, the ownership of the per-domain directory is the user to run the QEMU process and the user has the write permission of the directory. If VM escape occurs, the attacker can 1. write arbitrary content to the pid file (if running QEMU using root), then the attacker can kill any process by writing appropriate pid to the pid file; 2. spoof the pid file (if running QEMU using a regular user), then the pr-helper process will never be cleared even if the VM is destroyed.
So, move the pid file of pr-helper from per-domain directory to stateDir just like the pid file of the domain.
Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/qemu/qemu_process.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1d0165af6daa..583f3ec76c7b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2859,9 +2859,11 @@ static char * qemuProcessBuildPRHelperPidfilePath(virDomainObj *vm) { qemuDomainObjPrivate *priv = vm->privateData; - const char *prdAlias = qemuDomainGetManagedPRAlias(); + g_autofree char *domname = virDomainDefGetShortName(vm->def); + g_autofree char *prdName = g_strdup_printf("%s-%s", domname, qemuDomainGetManagedPRAlias()); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver);
- return virPidFileBuildPath(priv->libDir, prdAlias); + return virPidFileBuildPath(cfg->stateDir, prdName); }
I don't think it is this simple. The pidfile path is not stored in status XML but generated when starting/stopping PR helper process. Therefore, if there is PR helper that was started with older libvirt its pidfile is /var/lib/libvirt/qemu/domain-1-fedora/pr-helper0.pid but after your change a different path is generated /var/run/libvirt/qemu/1-fedora-pr-helper0.pid and thus qemuProcessKillManagedPRDaemon() does not kill the PR helper process and leaves it behind.
One solution would be to start recording the pidfile path in status XML and then when no path is found during parse we know that the old path was used.
Can we just change to use old path if the path returned by qemuProcessBuildPRHelperPidfilePath does not exist? For example: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f95ed80fac43..0ee1a97f5571 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2877,6 +2877,14 @@ qemuProcessKillManagedPRDaemon(virDomainObj *vm) return; }
+ if (!virFileExists(pidfile)) { + g_free(pidfile); + if (!(pidfile = qemuProcessBuildPRHelperPidfilePathOld(vm))) { + VIR_WARN("Unable to construct pr-helper pidfile path"); + return; + } + } + virErrorPreserveLast(&orig_err); if (virPidFileForceCleanupPath(pidfile) < 0) { VIR_WARN("Unable to kill pr-helper process");
qemuProcessBuildPRHelperPidfilePathOld will build the old path (e.g., /var/lib/libvirt/qemu/domain-1-fedora/pr-helper0.pid).
Yes, I think this may help. Michal
Libvirt will put the pid file of virtiofsd to per-domain directory. However, the ownership of the per-domain directory is the user to run the QEMU process and the user has the write permission of the directory. If VM escape occurs, the attacker can 1. write arbitrary content to the pid file (if running QEMU using root), then the attacker can kill any process by writing appropriate pid to the pid file; 2. spoof the pid file (if running QEMU using a regular user), then the virtiofsd process will never be cleared even if the VM is destroyed. So, move the pid file of virtiofsd from per-domain directory to stateDir just like the pid file of the domain. Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- src/qemu/qemu_virtiofs.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c index 08a8b4ed42a9..e617bb65fae0 100644 --- a/src/qemu/qemu_virtiofs.c +++ b/src/qemu/qemu_virtiofs.c @@ -44,11 +44,11 @@ qemuVirtioFSCreatePidFilename(virDomainObj *vm, const char *alias) { qemuDomainObjPrivate *priv = vm->privateData; - g_autofree char *name = NULL; + g_autofree char *domname = virDomainDefGetShortName(vm->def); + g_autofree char *name = g_strdup_printf("%s-%s-fs", domname, alias); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); - name = g_strdup_printf("%s-fs", alias); - - return virPidFileBuildPath(priv->libDir, name); + return virPidFileBuildPath(cfg->stateDir, name); } -- 2.31.1
participants (2)
-
Michal Prívozník -
Peng Liang