From: Marc-André Lureau <marcandre.lureau@redhat.com>
When swtpm reports "nvram-backend-dir", it can accepts a single file or
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.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
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)
--
2.45.2.827.g557ae147e6On Tue, Sep 10, 2024 at 11:06:00AM +0400, marcandre.lureau@redhat.com wrote:
>From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>When swtpm reports "nvram-backend-dir", it can accepts a single file or
>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.
>
>Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>---
> 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,
Should we also check that if that file is a block device that it is "big
enough"? Even though I'm not sure if we know what "big enough" means.
If the size depends on the guest, then of course there is no point in
doing that, I'm just wondering if we could avoid some errors by
reporting an issue earlier.
>@@ -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:
This suggests we should rather default to "dir" and not have the third
case (actually named "default") here.
>+ virCommandAddArgFormat(cmd, "dir=%s,mode=0600",
>+ tpm->data.emulator.storagepath);
>+ break;
>+ }
>
> virCommandAddArg(cmd, "--log");
> if (tpm->data.emulator.debug != 0)
>--
>2.45.2.827.g557ae147e6
>
On 9/10/24 3:06 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> 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 ...
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
> 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)
Hi
On Fri, Sep 13, 2024 at 5:14 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 9/10/24 3:06 AM, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > 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@redhat.com>
>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
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)
>
On 9/23/24 2:37 AM, Marc-André Lureau wrote:
> Hi
>
> On Fri, Sep 13, 2024 at 5:14 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>
>>
>> On 9/10/24 3:06 AM, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> 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?
Those guys who contributed the file backend didn't seem to need it. If
the file paths per VM are separated you don't really need. Otherwise it
may again make the coordination during migration over shared storage
more complicated.
>
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>
> 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)
>>
>
© 2016 - 2026 Red Hat, Inc.