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

[Peng Liang]

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 at 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
> 
> .
>