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

[Stefan Berger]

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