Re: [libvirt] [PATCH v2 2/6] tpm: Add support for external swtpm TPM emulator

[...] >>> >> Two blank lines between new functions and we like Free instead of Delete >> unless of course this is something more specific... > This is something more specifc. The TPM emulator writes state into files > in a dedicated directory. That state would normally be written into > NVRAM of a TPM so that, after it is powered up again, it has that old > state again (keys etc.) . We only delete this state and the per-VM > directory tree when the domain is undefined. > Probably just an API naming thing as I was reading forward... Lot of context to plow through on the first pass for me! >> [...] >>> @@ -27548,6 +27587,10 @@ virDomainDeleteConfig(const char *configDir, >>> goto cleanup; >>> } >>> + /* in case domain is NOT running, remove any TPM storage */ >>> + if (!dom->persistent) >> Well this has less to do with running and more to do with persistence >> vs. transient > Iirc this comes from the following: I ran into this case where I had a > running domain and undefined it. The domain keeps running of course, the > XML definition is gone. In this case we cannot just delete the TPM > emulator's storage while it is running but we have to do it later once > that VM terminates. > I wonder if this is something where using the /var/run/libvirt (or whatever the temporary running domain path is - my brain is fried from so many reviews)... Using this would cause/allow cleanup when the process eventually dies

>>> + virDomainTPMDelete(dom->def); >>> + [...] >>> virQEMUDriverConfigNew(bool privileged) >>> &cfg->nfirmwares) < 0) >>> goto error; >>> + if (virGetUserID("tss", &cfg->swtpm_user) < 0) >>> + cfg->swtpm_user = 0; /* root */ >>> + >> Something doesn't feel right here especially for unprivileged mode. > I haven't tried it in unprivileged mode. Not sure how to invoke it even... > -c qemu:///session

>> If there isn't a "tss" user, then are things configured correctly? > There are only two choice, one is tss and the other one is root. > >> Should we follow the other callers to virGetUserID and set to -1 instead? > Set swtpm_user = -1; ? What happens when we need to actually use this > variable then ? Report an error ? > When using the virFile* API's if the incoming uid/gid is -1, then it's not specifically and it IIRC it takes whatever the current default running mode is. If it is set, then it's set specifically to the value. Been a while since I traversed through all that code.

>> Still 0 is the default for @cfg members anyway. >> [...] >>> @@ -7510,6 +7514,9 @@ qemuDomainUndefineFlags(virDomainPtr dom, >>> if (!(vm = qemuDomObjFromDomain(dom))) >>> return -1; >>> + if (qemuExtDevicesInitPaths(driver, vm->def) < 0) >>> + return -1; >> Again, confused over failure in an undefine path? Or is this just git >> diff fooling me? > This is for a domain that's being undefined but was never started (which > would have caused the path initialization) since libvirtd was restarted. > Again, we need to initialize the paths. If there was a better place to > put these paths, I'd be curious where that is. > Perhaps "stateDir" - it just came to me - hopefully it's what I was thinking about ;-) [...] >>> +{ >>> + struct dirent *ent; >>> + int ret; >>> + DIR *dir; >>> + char *path; >>> + >>> + if (virDirOpen(&dir, name) < 0) >>> + return -1; >> Hope that @name isn't a file path on an NFS volume - there's some other > That would depend on the configuration of the host > >> consumers of chown in this module that you may want to have a look at. > The other consumers stat the file before and check whether chown() is > necessary. Hm, is that the recommended way. I just try to change all of > them without looking at the state. > The NFS thing is something that came to mind while thinking of uid/gid mgmt and rootsquash mode (or something like that) which absolutely causes nightmares w/r/t using root...

>>> + >>> + while ((ret = virDirRead(dir, &ent, name)) > 0) { >>> + if (ent->d_type != DT_REG) >>> + continue; >>> + >>> + if (virAsprintf(&path, "%s/%s", name, ent->d_name) < 0) { >>> + ret = -1; >>> + break; >>> + } >>> + if (chown(path, uid, gid) < 0) { >> If uid/gid change from non root user X to user Y, then wouldn't this >> just fail anyway? > Why would it fail if root does that? libvirtd runs as root. > Guess I was thinking of session mode or unprivileged mode.

>> Still using the -1, -1 logic like other chown/chmod consumers may >> actually be a benefit here at least w/r/t not failing. > Not sure what you mean. uid = -1 passed to chown() means, don't change > it. Same for gid = -1. > > I think I redescribed that a bit above. It wasn't fresh in my mind, but essentially looking at the various virFile API's that would check uid/gid vs. -1 and thinking how qemu.conf and storage pool <permissions> used -1, -1... Again the details escape me at the moment. [...] >>> +static int >>> +virTPMCreateEmulatorStorage(const char *storagepath, >>> + bool *created, >>> + uid_t swtpm_user) >>> +{ >>> + int ret = -1; >>> + char *swtpmStorageDir = virTPMGetTPMStorageDir(storagepath); >>> + >>> + if (!swtpmStorageDir) >>> + return -1; >>> + >>> + if (virTPMEmulatorInitStorage(swtpmStorageDir) < 0) >>> + return -1; >>> + >>> + *created = false; >>> + >>> + if (!virFileExists(storagepath)) >>> + *created = true;> + >>> + if (virDirCreate(storagepath, 0700, swtpm_user, swtpm_user, >> IIRC: We only checked if swtpm_user was a user - we did not check if it >> was also a gid... You cannot use the same value for both, can you? > You are right. I need a swtpm_group, eh ? On my system tss happens to be > mapped to 59 for uid and gid. > >> Still may want to consider using the -1, -1 because it is handled via >> the File API's. > -1 uid and -1 gid passed to chown() would mean don't change it. > One problem is that a user may edit /etc/libvirt/qemu.conf and change > this line here from root to tss: > > swtpm_user = "tss" > > In that case all file ownerships have to be adjusted so that the swtpm > can access the files. That's why I am always adjusting the ownerships > before swtpm or the other tools are started. > I suspect by this time you've dug in a bit more and perhaps have more details than I can recall at the moment.

[...] > I broke this patch up into several patches. Thanks for looking at it. > > Stefan Thanks... I know it's painful from experience ;-)

John