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@xxxxxxx>
> ---
> 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
