On 04/26/2018 08:07 PM, John Ferlan wrote:
[...]
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
For this persistent state I am now using /var/lib/libvirt/swtpm; it
seems better than /var/run/...
+ 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
Ok, thanks. I tried that now and fixed a few things. swtpm_setup needs
high privileges to run (or be run as 'tss' user), so it won't run when
in session mode.
What is also a bit of a concern is the SELinux svirt support. I have the
following rules I need to apply with the 3rd one now to support session
mode, the 4th one added for FC27 (previously only run on FC23):
allow svirt_t virtd_t:fifo_file write;
allow svirt_t virtd_t:process sigchld;
allow svirt_t user_tmp_t:sock_file { create setattr };
allow svirt_t swtpm_exec_t:file entrypoint;
https://github.com/stefanberger/swtpm/blob/tpm2-preview.v2/src/selinux/swtpm_svirt.te
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.
-1 seems to mean to not change anything. I initialize swtpm_user and
swtpm_group now to -1 when in session mode. It works.
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...
Hm, not sure what to do about this. Document it? Prevent root user from
being used ?
+
+ 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.
Right, hadn't used that before. Makes sense now.
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.
there's s swtpm_group = "tss" now as well.
[...]
I broke this patch up into several patches. Thanks for looking at it.
Stefan
Thanks... I know it's painful from experience ;-)
Not so bad when one can cut and paste whole files form a patch into a
new patch.
John
Stefan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
