[PATCH v4 6/7] qemu: tpm: Avoid security labels on incoming migration with shared storage

[Stefan Berger]

On 11/29/22 09:04, Michal Prívozník wrote:
> On 10/24/22 12:28, Stefan Berger wrote:
>> When using shared storage there is no need to apply security labels on the
>> storage since the files have to have been labeled already on the source
>> side and we must assume that the source and destination side have been
>> setup to use the same uid and gid for running swtpm as well as share the
>> same security labels. Whether the security labels can be used at all
>> depends on the shared storage and whether and how it supports them.
>>
>> Signed-off-by: Stefan Berger <stefanb at linux.ibm.com>
>> ---
>>   src/qemu/qemu_tpm.c | 17 +++++++++++++----
>>   1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
>> index cffa77cfa3..5a0d298052 100644
>> --- a/src/qemu/qemu_tpm.c
>> +++ b/src/qemu/qemu_tpm.c
>> @@ -932,10 +932,19 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
>>       virCommandSetPidFile(cmd, pidfile);
>>       virCommandSetErrorFD(cmd, &errfd);
>>   
>> -    if (qsudo tpm2_nvdefine -C o -a "nt=extend|ownerread|ownerwrite|policywrite|writedefine|orderly" 2(driver, vm, cmd,
>> -                                     cfg->swtpm_user, cfg->swtpm_group,
>> -                                     NULL, &cmdret) < 0)
>> -        return -1;
>> +    if (incomingMigration &&
>> +        virFileIsSharedFS(tpm->data.emulator.storagepath) == 1) {
>> +        /* security labels must have been set up on source already */
>> +        if (qemuSecurityCommandRun(driver, vm, cmd,
>> +                                   cfg->swtpm_user, cfg->swtpm_group,
>> +                                   NULL, &cmdret) < 0) {
>> +            goto error;
>> +        }
>> +    } else if (qemuSecurityStartTPMEmulator(driver, vm, cmd,
>> +                                            cfg->swtpm_user, cfg->swtpm_group,
>> +                                            NULL, &cmdret) < 0) {
>> +        goto error;
>> +    }
>>   
>>       if (cmdret < 0) {
>>           /* virCommandRun() hidden in qemuSecurityStartTPMEmulator()
> 
> Turns out, this is patch is problematic with SELinux [1]. Couple of
> problems here:
> 
> 1) When a domain is being started up, libvirt creates an unique SELinux
> label. This is not migrated onto the destination - here another unique
> label is created on incoming migration. This means that the state might
> be not available to the destination swtpm binary.
> 
> 2) the log file should be relabelled regardless - it's not on the shared
> volume. And since its label is set in qemuSecurityStartTPMEmulator(), it
> won't be set on the destination.
> 
> 
> While I could deal with 2), I am not sure what to do with 1). Because
> when I tried to set the label (basically by reverting this patch), the
> destination was unhappy as it could not lock the .lock file.
> 
> Stefan, do you have any bright idea how to fix this?

Sorry, no, unfortunately I don't.
SELinux and NFS had some issues and due to other reasons as well I had to run my machine in permissive mode but I don't remember the exact details. What I at least was not able to do was to run qemuSecurityStartTPMEmulator on the destination side - this didn't work just to begin with. Also, NFS is one example of a shared filesystem and I am not sure what issues other shared filesystems may or may not have when SELinux is used -- could a shared filesystem entertain local SELinux labels and not have the issue that NFS has with SELinux labels? In the worst case we may have to put a blocker into the code if SELinux(+NFS) is being used. Migration across shared storage shouldn't be a problem with AppArmor at least.

     Stefan

> 
> 1: https://bugzilla.redhat.com/show_bug.cgi?id=2130192
> 
> Michal
>