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

[Michal Prívozník]

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

Yes, I think this may help.

Michal