Re: [PATCH 1/2] qemu: Move pid file of pr-helper to stateDir

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(a)huawei.com&gt; >> --- >> 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).