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@xxxxxxxxxx>
>> ---
>> 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
>
> .
>
