Hi
On Fri, Sep 13, 2024 at 5:14 PM Stefan Berger <stefanb@xxxxxxxxxxxxx> wrote:
>
>
>
> On 9/10/24 3:06 AM, marcandre.lureau@xxxxxxxxxx wrote:
> > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> >
> > When swtpm reports "nvram-backend-dir", it can accepts a single file or
>
> s/accepts/accept
>
> > block device where TPM state will be stored. --tpmstate must be
> > backend-uri=file://<path>.
> >
> > Teach the storage to use custom directory or file source location.
>
> Is it a concern that the file backend does not lock access to the TPM
> state file like the dir backend does? The dir backend would prevent
> concurrent usage of the state by two different instances of swtpm even
> now that the user gets control over the file paths but the file backend
> will not prevent it ...
>
Why not use a lock on the file too?
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
>
> Reviewed-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
thanks
>
> > ---
> > src/qemu/qemu_tpm.c | 76 +++++++++++++++++++++++++++++++++++++--------
> > 1 file changed, 63 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> > index 2f17918cbb..faf07556d2 100644
> > --- a/src/qemu/qemu_tpm.c
> > +++ b/src/qemu/qemu_tpm.c
> > @@ -340,9 +340,28 @@ qemuTPMVirCommandAddEncryption(virCommand *cmd,
> > }
> >
> >
> > +static char *
> > +qemuTPMGetSwtpmSetupStateArg(const virDomainTPMStorage storage_type,
> > + const char *storagepath)
> > +{
> > + switch (storage_type) {
> > + case VIR_DOMAIN_TPM_STORAGE_FILE:
> > + /* the file:// prefix is supported since swtpm_setup 0.7.0 */
> > + /* assume the capability check for swtpm is redundant. */
> > + return g_strdup_printf("file://%s", storagepath);
> > + case VIR_DOMAIN_TPM_STORAGE_DIR:
> > + case VIR_DOMAIN_TPM_STORAGE_DEFAULT:
> > + return g_strdup_printf("%s", storagepath);
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +
> > /*
> > * qemuTPMEmulatorRunSetup
> > *
> > + * @storage_type: type of storage
> > * @storagepath: path to the directory for TPM state
> > * @vmname: the name of the VM
> > * @vmuuid: the UUID of the VM
> > @@ -360,7 +379,8 @@ qemuTPMVirCommandAddEncryption(virCommand *cmd,
> > * certificates for it.
> > */
> > static int
> > -qemuTPMEmulatorRunSetup(const char *storagepath,
> > +qemuTPMEmulatorRunSetup(const virDomainTPMStorage storage_type,
> > + const char *storagepath,
> > const char *vmname,
> > const unsigned char *vmuuid,
> > bool privileged,
> > @@ -376,6 +396,7 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
> > char uuid[VIR_UUID_STRING_BUFLEN];
> > g_autofree char *vmid = NULL;
> > g_autofree char *swtpm_setup = virTPMGetSwtpmSetup();
> > + g_autofree char *tpm_state = qemuTPMGetSwtpmSetupStateArg(storage_type, storagepath);
> >
> > if (!swtpm_setup)
> > return -1;
> > @@ -413,7 +434,7 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
> >
> > if (!incomingMigration) {
> > virCommandAddArgList(cmd,
> > - "--tpm-state", storagepath,
> > + "--tpm-state", tpm_state,
> > "--vmid", vmid,
> > "--logfile", logfile,
> > "--createek",
> > @@ -424,7 +445,7 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
> > NULL);
> > } else {
> > virCommandAddArgList(cmd,
> > - "--tpm-state", storagepath,
> > + "--tpm-state", tpm_state,
> > "--logfile", logfile,
> > "--overwrite",
> > NULL);
> > @@ -465,6 +486,7 @@ qemuTPMPcrBankBitmapToStr(virBitmap *activePcrBanks)
> > * qemuTPMEmulatorReconfigure
> > *
> > *
> > + * @storage_type: type of storage
> > * @storagepath: path to the directory for TPM state
> > * @swtpm_user: The userid to switch to when setting up the TPM;
> > * typically this should be the uid of 'tss' or 'root'
> > @@ -478,7 +500,8 @@ qemuTPMPcrBankBitmapToStr(virBitmap *activePcrBanks)
> > * Reconfigure the active PCR banks of a TPM 2.
> > */
> > static int
> > -qemuTPMEmulatorReconfigure(const char *storagepath,
> > +qemuTPMEmulatorReconfigure(const virDomainTPMStorage storage_type,
> > + const char *storagepath,
> > uid_t swtpm_user,
> > gid_t swtpm_group,
> > virBitmap *activePcrBanks,
> > @@ -490,6 +513,7 @@ qemuTPMEmulatorReconfigure(const char *storagepath,
> > int exitstatus;
> > g_autofree char *activePcrBanksStr = NULL;
> > g_autofree char *swtpm_setup = virTPMGetSwtpmSetup();
> > + g_autofree char *tpm_state = qemuTPMGetSwtpmSetupStateArg(storage_type, storagepath);
> >
> > if (!swtpm_setup)
> > return -1;
> > @@ -510,7 +534,7 @@ qemuTPMEmulatorReconfigure(const char *storagepath,
> > return -1;
> >
> > virCommandAddArgList(cmd,
> > - "--tpm-state", storagepath,
> > + "--tpm-state", tpm_state,
> > "--logfile", logfile,
> > "--pcr-banks", activePcrBanksStr,
> > "--reconfigure",
> > @@ -555,6 +579,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm,
> > {
> > g_autoptr(virCommand) cmd = NULL;
> > bool created = false;
> > + bool run_setup = false;
> > g_autofree char *swtpm = virTPMGetSwtpm();
> > int pwdfile_fd = -1;
> > int migpwdfile_fd = -1;
> > @@ -565,6 +590,18 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm,
> > if (!swtpm)
> > return NULL;
> >
> > + if (tpm->data.emulator.storage_type == VIR_DOMAIN_TPM_STORAGE_FILE) {
> > + if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_NVRAM_BACKEND_DIR)) {
> > + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
> > + _("%1$s does not support file storage"),
> > + swtpm);
> > + goto error;
> > + }
> > + create_storage = false;
> > + /* setup is run with --not-overwrite */
> > + run_setup = true;
> > + }
> > +
> > /* Do not create storage and run swtpm_setup on incoming migration over
> > * shared storage
> > */
> > @@ -572,15 +609,18 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm,
> > if (incomingMigration && on_shared_storage)
> > create_storage = false;
> >
> > - if (create_storage &&
> > - qemuTPMEmulatorCreateStorage(tpm, &created, swtpm_user, swtpm_group) < 0)
> > - return NULL;
> > + if (create_storage) {
> > + if (qemuTPMEmulatorCreateStorage(tpm, &created, swtpm_user, swtpm_group) < 0)
> > + return NULL;
> > + run_setup = created;
> > + }
> >
> > if (tpm->data.emulator.hassecretuuid)
> > secretuuid = tpm->data.emulator.secretuuid;
> >
> > - if (created &&
> > - qemuTPMEmulatorRunSetup(tpm->data.emulator.storagepath, vmname, vmuuid,
> > + if (run_setup &&
> > + qemuTPMEmulatorRunSetup(tpm->data.emulator.storage_type,
> > + tpm->data.emulator.storagepath, vmname, vmuuid,
> > privileged, swtpm_user, swtpm_group,
> > tpm->data.emulator.logfile,
> > tpm->data.emulator.version,
> > @@ -588,7 +628,8 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm,
> > goto error;
> >
> > if (!incomingMigration &&
> > - qemuTPMEmulatorReconfigure(tpm->data.emulator.storagepath,
> > + qemuTPMEmulatorReconfigure(tpm->data.emulator.storage_type,
> > + tpm->data.emulator.storagepath,
> > swtpm_user, swtpm_group,
> > tpm->data.emulator.activePcrBanks,
> > tpm->data.emulator.logfile,
> > @@ -607,8 +648,17 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm,
> > tpm->data.emulator.source->data.nix.path);
> >
> > virCommandAddArg(cmd, "--tpmstate");
> > - virCommandAddArgFormat(cmd, "dir=%s,mode=0600",
> > - tpm->data.emulator.storagepath);
> > + switch (tpm->data.emulator.storage_type) {
> > + case VIR_DOMAIN_TPM_STORAGE_FILE:
> > + virCommandAddArgFormat(cmd, "backend-uri=file://%s",
> > + tpm->data.emulator.storagepath);
> > + break;
> > + case VIR_DOMAIN_TPM_STORAGE_DIR:
> > + case VIR_DOMAIN_TPM_STORAGE_DEFAULT:
> > + virCommandAddArgFormat(cmd, "dir=%s,mode=0600",
> > + tpm->data.emulator.storagepath);
> > + break;
> > + }
> >
> > virCommandAddArg(cmd, "--log");
> > if (tpm->data.emulator.debug != 0)
>
