The function will attempt to read a pid from @path, and store it in @pid. The @pid will only be set, however, if @path is locked by virFileLock() at byte 0 and the pid in @path is running. Signed-off-by: Vasiliy Ulyanov <vulyanov(a)suse.de&gt; --- src/libvirt_private.syms | 1 + src/util/virpidfile.c | 34 ++++++++++++++++++++++++++++++++++ src/util/virpidfile.h | 2 ++ 3 files changed, 37 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9bc3d9530b..447ba9d82b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3070,6 +3070,7 @@ virPidFileRead; virPidFileReadIfAlive; virPidFileReadPath; virPidFileReadPathIfAlive; +virPidFileReadPathIfLocked; virPidFileRelease; virPidFileReleasePath; virPidFileWrite; diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index 7069f8343d..b8bb455e5e 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -302,6 +302,40 @@ int virPidFileReadIfAlive(const char *dir, return 0; } +/** + * virPidFileReadPathIfLocked: + * @path: path to pidfile + * @pid: variable to return pid in + * + * This will attempt to read a pid from @path, and store it + * in @pid. The @pid will only be set, however, if the + * pid in @path is running, and @path is locked by virFileLock() + * at byte 0. This adds protection against returning a stale pid. + * + * Returns -1 upon error, or zero on successful + * reading of the pidfile. If @path is not locked + * or if the PID was not still alive, zero will + * be returned, but @pid will be set to -1. + */ +int virPidFileReadPathIfLocked(const char *path, pid_t *pid) +{ + VIR_AUTOCLOSE fd = -1; + + if ((fd = open(path, O_RDWR)) < 0) + return -1; + + if (virFileLock(fd, false, 0, 1, false) >= 0) { + /* The file isn't locked. PID is stale. */ + *pid = -1; + return 0; + } + + if (virPidFileReadPathIfAlive(path, pid, NULL) < 0) + return -1; + + return 0; +} + int virPidFileDeletePath(const char *pidfile) { diff --git a/src/util/virpidfile.h b/src/util/virpidfile.h index fd8013c41e..e84542f298 100644 --- a/src/util/virpidfile.h +++ b/src/util/virpidfile.h @@ -48,6 +48,8 @@ int virPidFileReadIfAlive(const char *dir, const char *name, pid_t *pid, const char *binpath) G_GNUC_WARN_UNUSED_RESULT; +int virPidFileReadPathIfLocked(const char *path, + pid_t *pid) G_GNUC_WARN_UNUSED_RESULT; int virPidFileDeletePath(const char *path); int virPidFileDelete(const char *dir, -- 2.34.1

Access to /proc/[pid]/exe may be restricted in certain environments (e.g. in containers) and any attempt to stat(2) or readlink(2) the file will result in 'permission denied' error if the calling process does not have CAP_SYS_PTRACE capability. According to proc(5) manpage: Permission to dereference or read (readlink(2)) this symbolic link is governed by a ptrace access mode PTRACE_MODE_READ_FSCREDS check; see ptrace(2). The binary validation in virPidFileReadPathIfAlive may fail with EACCES. Therefore instead do only the check that the pidfile is locked by the correct process. To ensure this is always the case the daemonization and pidfile handling of the swtpm command is now controlled by libvirt. Signed-off-by: Vasiliy Ulyanov <vulyanov(a)suse.de&gt; --- src/qemu/qemu_tpm.c | 40 +++++++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 7e7b01768e..47c7891a4f 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -258,13 +258,13 @@ qemuTPMEmulatorGetPid(const char *swtpmStateDir, const char *shortName, pid_t *pid) { - g_autofree char *swtpm = virTPMGetSwtpm(); g_autofree char *pidfile = qemuTPMEmulatorCreatePidFilename(swtpmStateDir, shortName); + if (!pidfile) return -1; - if (virPidFileReadPathIfAlive(pidfile, pid, swtpm) < 0) + if (virPidFileReadPathIfLocked(pidfile, pid) < 0) return -1; return 0; @@ -721,7 +721,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, virCommandClearCaps(cmd); - virCommandAddArgList(cmd, "socket", "--daemon", "--ctrl", NULL); + virCommandAddArgList(cmd, "socket", "--ctrl", NULL); virCommandAddArgFormat(cmd, "type=unixio,path=%s,mode=0600", tpm->data.emulator.source->data.nix.path); @@ -751,8 +751,8 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, if (!(pidfile = qemuTPMEmulatorCreatePidFilename(swtpmStateDir, shortName))) goto error; - virCommandAddArg(cmd, "--pid"); - virCommandAddArgFormat(cmd, "file=%s", pidfile); + virCommandSetPidFile(cmd, pidfile); + virCommandDaemonize(cmd); if (tpm->data.emulator.hassecretuuid) { if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_CMDARG_PWD_FD)) { @@ -905,7 +905,7 @@ qemuExtTPMStartEmulator(virQEMUDriver *driver, { g_autoptr(virCommand) cmd = NULL; int exitstatus = 0; - g_autofree char *errbuf = NULL; + VIR_AUTOCLOSE errfd = -1; g_autoptr(virQEMUDriverConfig) cfg = NULL; g_autofree char *shortName = virDomainDefGetShortName(vm->def); int cmdret = 0, timeout, rc; @@ -930,7 +930,7 @@ qemuExtTPMStartEmulator(virQEMUDriver *driver, if (qemuExtDeviceLogCommand(driver, vm, cmd, "TPM Emulator") < 0) return -1; - virCommandSetErrorBuffer(cmd, &errbuf); + virCommandSetErrorFD(cmd, &errfd); if (qemuSecurityStartTPMEmulator(driver, vm, cmd, cfg->swtpm_user, cfg->swtpm_group, @@ -938,23 +938,33 @@ qemuExtTPMStartEmulator(virQEMUDriver *driver, return -1; if (cmdret < 0 || exitstatus != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not start 'swtpm'. exitstatus: %d, " - "error: %s"), exitstatus, errbuf); + char errbuf[1024] = { 0 }; + + if (saferead(errfd, errbuf, sizeof(errbuf) - 1) < 0) { + virReportSystemError(errno, + _("Could not start 'swtpm'. exitstatus: %d"), + exitstatus); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not start 'swtpm'. exitstatus: %d, " + "error: %s"), exitstatus, errbuf); + } + return -1; } - /* check that the swtpm has written its pid into the file */ + /* check that the swtpm has written its pid into the file and the control + * socket has been created. */ + rc = qemuTPMEmulatorGetPid(cfg->swtpmStateDir, shortName, &pid); + if ((rc == 0 && pid == (pid_t)-1) || rc < 0) + goto error; timeout = 1000; /* ms */ while (timeout > 0) { - rc = qemuTPMEmulatorGetPid(cfg->swtpmStateDir, shortName, &pid); - if (rc < 0) { + if (!virFileExists(tpm->data.emulator.source->data.nix.path)) { timeout -= 50; g_usleep(50 * 1000); continue; } - if (rc == 0 && pid == (pid_t)-1) - goto error; break; } if (timeout <= 0) -- 2.34.1

The binary validation in virPidFileReadPathIfAlive may fail with EACCES if the calling process does not have CAP_SYS_PTRACE capability. Therefore instead do only the check that the pidfile is locked by the correct process. Fixes the same issue as with swtpm. Signed-off-by: Vasiliy Ulyanov <vulyanov(a)suse.de&gt; --- src/qemu/qemu_vhost_user_gpu.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_vhost_user_gpu.c b/src/qemu/qemu_vhost_user_gpu.c index ef198a4820..94e758f78d 100644 --- a/src/qemu/qemu_vhost_user_gpu.c +++ b/src/qemu/qemu_vhost_user_gpu.c @@ -20,6 +20,8 @@ #include <config.h> +#include <fcntl.h> + #include "qemu_vhost_user_gpu.h" #include "qemu_vhost_user.h" #include "qemu_extdevice.h" @@ -54,7 +56,6 @@ qemuVhostUserGPUCreatePidFilename(const char *stateDir, /* * qemuVhostUserGPUGetPid: - * @binpath: path of executable associated with the pidfile * @stateDir: the directory where vhost-user-gpu writes the pidfile into * @shortName: short name of the domain * @alias: video device alias @@ -65,8 +66,7 @@ qemuVhostUserGPUCreatePidFilename(const char *stateDir, * set to -1; */ static int -qemuVhostUserGPUGetPid(const char *binPath, - const char *stateDir, +qemuVhostUserGPUGetPid(const char *stateDir, const char *shortName, const char *alias, pid_t *pid) @@ -76,7 +76,7 @@ qemuVhostUserGPUGetPid(const char *binPath, if (!(pidfile = qemuVhostUserGPUCreatePidFilename(stateDir, shortName, alias))) return -1; - if (virPidFileReadPathIfAlive(pidfile, pid, binPath) < 0) + if (virPidFileReadPathIfLocked(pidfile, pid) < 0) return -1; return 0; @@ -253,8 +253,7 @@ qemuExtVhostUserGPUSetupCgroup(virQEMUDriver *driver, if (!shortname) return -1; - rc = qemuVhostUserGPUGetPid(video->driver->vhost_user_binary, - cfg->stateDir, shortname, video->info.alias, &pid); + rc = qemuVhostUserGPUGetPid(cfg->stateDir, shortname, video->info.alias, &pid); if (rc < 0 || (rc == 0 && pid == (pid_t)-1)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not get process id of vhost-user-gpu")); -- 2.34.1