[PATCH v3 0/3] qemu_tpm: Get swtpm pid without binary validation
[v1] https://listman.redhat.com/archives/libvir-list/2022-January/msg00008.html [v2] https://listman.redhat.com/archives/libvir-list/2022-January/msg00582.html As suggesed in the review comments: - dropped virFileGetLockOwner; - simplified lock validation by using VIR_AUTOCLOSE and just trying to lock the file; - introduced virPidFileReadPathIfLocked to preserve the existing behaviour of virPidFileReadPathIfAlive. Vasiliy Ulyanov (3): virpidfile: Add virPidFileReadPathIfLocked func qemu: tpm: Get swtpm pid without binary validation qemu: gpu: Get pid without binary validation src/libvirt_private.syms | 1 + src/qemu/qemu_tpm.c | 40 +++++++++++++++++++++------------- src/qemu/qemu_vhost_user_gpu.c | 11 +++++----- src/util/virpidfile.c | 34 +++++++++++++++++++++++++++++ src/util/virpidfile.h | 2 ++ 5 files changed, 67 insertions(+), 21 deletions(-) -- 2.34.1
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@suse.de> --- 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
On 2/2/22 17:28, Vasiliy Ulyanov wrote:
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@suse.de> --- 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.
I'd mention here virCommandSetPidFile(), but otherwise looking good. Michal
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@suse.de> --- 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
On 2/2/22 17:28, Vasiliy Ulyanov wrote:
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@suse.de> --- 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); + } +
This doesn't do what you think it does. At this point we are not guaranteed that the child (swtpm) even entered its main(). All we know that the double-fork() succeeded and the PID of the swtpm process is in the @pidfile. Therefore @errfd must be read only after we've learned that the process died and for that we must firstly read the pidfile. Therefore, what we should do here is read the pidfile to learn the PID, but don't use virPidFileReadPathIfLocked() just yet. The reason is that we want to enter the while() loop even if PID doesn't exist anymore, because @errfd is read there.
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)) {
Here, the loop has to look a bit differently. If the socket was created we know that the swtpm started and can break from the loop. If the socket doesn't exist, then we have to check whether the process still exists (in that case it is still initializing itself, maybe parsing args, who knows). But in that case we can sleep for a while and continue to the next iteration. If neither socket nor process exist, then we can finally read @errfd to learn error message reported by the binary. Maybe it didn't report anything in which case we can report generic message. What I am trying to say is, follow the example of qemuProcessStartManagedPRDaemon().
timeout -= 50; g_usleep(50 * 1000); continue; } - if (rc == 0 && pid == (pid_t)-1) - goto error; break; } if (timeout <= 0)
Michal
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@suse.de> --- 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
On 2/2/22 17:28, Vasiliy Ulyanov wrote:
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@suse.de> --- 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> +
This doesn't look necessary. Michal
On 2/2/22 17:28, Vasiliy Ulyanov wrote:
[v1] https://listman.redhat.com/archives/libvir-list/2022-January/msg00008.html [v2] https://listman.redhat.com/archives/libvir-list/2022-January/msg00582.html
As suggesed in the review comments: - dropped virFileGetLockOwner; - simplified lock validation by using VIR_AUTOCLOSE and just trying to lock the file; - introduced virPidFileReadPathIfLocked to preserve the existing behaviour of virPidFileReadPathIfAlive.
Vasiliy Ulyanov (3): virpidfile: Add virPidFileReadPathIfLocked func qemu: tpm: Get swtpm pid without binary validation qemu: gpu: Get pid without binary validation
src/libvirt_private.syms | 1 + src/qemu/qemu_tpm.c | 40 +++++++++++++++++++++------------- src/qemu/qemu_vhost_user_gpu.c | 11 +++++----- src/util/virpidfile.c | 34 +++++++++++++++++++++++++++++ src/util/virpidfile.h | 2 ++ 5 files changed, 67 insertions(+), 21 deletions(-)
Apart from patch 2/3 this looks good. So here's what we are going to do. I've uploaded fixup commits onto my gitlab: https://gitlab.com/MichalPrivoznik/libvirt/-/commits/virpidfile/ Please take a look and if you agree I will squash those fixup commits and merge. Michal
Hi Michal, On 03/02/2022 18:09, Michal Prívozník wrote:
Apart from patch 2/3 this looks good. So here's what we are going to do. I've uploaded fixup commits onto my gitlab:
https://gitlab.com/MichalPrivoznik/libvirt/-/commits/virpidfile/
Please take a look and if you agree I will squash those fixup commits and merge.
Thank you for the detailed explanation in 2/3. Makes sense. Probably I need to dive a bit deeper into the code the next time. And yeah, I missed that redundant include initially and noticed it only after sending the patches. I agree on squashing the fixups and merging. -- Vasily Ulyanov <vulyanov@suse.de> Software Engineer, SUSE Labs Core
On 2/3/22 18:56, Vasily Ulyanov wrote:
Hi Michal,
On 03/02/2022 18:09, Michal Prívozník wrote:
Apart from patch 2/3 this looks good. So here's what we are going to do. I've uploaded fixup commits onto my gitlab:
https://gitlab.com/MichalPrivoznik/libvirt/-/commits/virpidfile/
Please take a look and if you agree I will squash those fixup commits and merge.
Thank you for the detailed explanation in 2/3. Makes sense. Probably I need to dive a bit deeper into the code the next time. And yeah, I missed that redundant include initially and noticed it only after sending the patches. I agree on squashing the fixups and merging.
Perfect, fixed and merged. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (3)
-
Michal Prívozník -
Vasiliy Ulyanov -
Vasily Ulyanov