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

[John Ferlan]

[...]

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