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@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).
Yes, I think this may help.
Michal
