On 5/4/26 3:12 AM, Arun Menon wrote:
On Thu, Apr 30, 2026 at 07:46:33AM -0400, Stefan Berger wrote:On 4/30/26 1:11 AM, Arun Menon wrote:On Wed, Apr 29, 2026 at 07:14:38PM -0400, Stefan Berger wrote:On 4/22/26 6:30 AM, Arun Menon wrote:From: Arun Menon <[email protected]> - Add subsection in VMState for TPM CRB with the newly introduced command and response buffer GByteArrays, along with a needed callback, so that newer QEMU only sends the buffers if it is necessary. - Implement a migration blocker to prevent migration of the VM if the user manually enables chunking capability, cap-chunk, but the machine type does not support it, using a new hw_compat property called allow_chunk_migration. - Add a post_load_errp hook so that during a migration, the buffers are validated before destination VM is started. Signed-off-by: Arun Menon <[email protected]> --- hw/core/machine.c | 1 + hw/tpm/tpm_crb.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/hw/core/machine.c b/hw/core/machine.c index 6d27cf69a2..b590af0125 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -40,6 +40,7 @@ GlobalProperty hw_compat_11_0[] = { { "tpm-crb", "cap-chunk", "off"}, + { "tpm-crb", "x-allow-chunk-migration", "off"}, }; const size_t hw_compat_11_0_len = G_N_ELEMENTS(hw_compat_11_0); diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c index 29370d6f49..23e6948aee 100644 --- a/hw/tpm/tpm_crb.c +++ b/hw/tpm/tpm_crb.c @@ -24,6 +24,7 @@ #include "hw/pci/pci_ids.h" #include "hw/acpi/tpm.h" #include "migration/vmstate.h" +#include "migration/blocker.h" #include "system/tpm_backend.h" #include "system/tpm_util.h" #include "system/reset.h" @@ -51,6 +52,8 @@ struct CRBState { TPMPPI ppi; bool cap_chunk; + bool allow_chunk_migration; + Error *migration_blocker; }; typedef struct CRBState CRBState; @@ -353,12 +356,63 @@ static int tpm_crb_pre_save(void *opaque) return 0; } +static bool tpm_crb_chunk_needed(void *opaque) +{ + CRBState *s = opaque; + + if (!s->allow_chunk_migration) { + return false; + } + + return ((s->command_buffer && s->command_buffer->len > 0) || + (s->response_buffer && s->response_buffer->len > 0)); +} + +static bool tpm_crb_chunk_post_load(void *opaque, int version_id, Error **errp) +{ + CRBState *s = opaque; + + if (!s->response_buffer || !s->command_buffer) { + error_setg(errp, "tpm-crb: Internal buffers are not allocated");Could this happen that this state type is resumed but the buffers are not allocated?Yes, this is not required. Its just a defense. The tpm_realize() function should allocate the command and response buffers.+ return false; + } + if (s->response_offset > s->response_buffer->len) { + error_setg(errp, "tpm-crb: Invalid response " + "offset %" PRIu32 " in migration stream", + s->response_offset); + return false; + }The check is correct but can this particular case occur other than through crafted input?Not that I can think of. The check is indeed added to address malicious migration stream / crafted input.I think that devices are assuming trusted input for all devices resumed from their previously stored state...I see. Shall I remove these two checks at the beginning and keep only the last one?
I think so. For the last one I think you should write a description what role the external emulator can play here.
+ if (s->response_buffer->len > s->be_buffer_size || + s->command_buffer->len > s->be_buffer_size) {I suppose this could happen if I was running with a PQC-enable swtpm/libtpms and suspended at the right moment that when a chunked command or response was in the buffer and now I am trying to resume with a non-PQC-enabled swtpm that only has 4kb buffer, while the newer PQC one had 8kb buffer.Yes. This check is for safety. We do not want to migrate to a VM that has a tpm backend that does not support big buffers.+ error_setg(errp, "tpm-crb: Buffer sizes exceed backend capacity"); + return false; + } + return true; +} + +static const VMStateDescription vmstate_tpm_crb_chunk = { + .name = "tpm-crb/chunk", + .version_id = 0, + .needed = tpm_crb_chunk_needed, + .post_load_errp = tpm_crb_chunk_post_load, + .fields = (const VMStateField[]) { + VMSTATE_GBYTEARRAY(command_buffer, CRBState, 0), + VMSTATE_GBYTEARRAY(response_buffer, CRBState, 0), + VMSTATE_UINT32(response_offset, CRBState), + VMSTATE_END_OF_LIST() + } +}; + static const VMStateDescription vmstate_tpm_crb = { .name = "tpm-crb", .pre_save = tpm_crb_pre_save, .fields = (const VMStateField[]) { VMSTATE_UINT32_ARRAY(regs, CRBState, TPM_CRB_R_MAX), VMSTATE_END_OF_LIST(), + }, + .subsections = (const VMStateDescription * const []) { + &vmstate_tpm_crb_chunk, + NULL, } }; @@ -366,6 +420,8 @@ static const Property tpm_crb_properties[] = { DEFINE_PROP_TPMBE("tpmdev", CRBState, tpmbe), DEFINE_PROP_BOOL("ppi", CRBState, ppi_enabled, true), DEFINE_PROP_BOOL("cap-chunk", CRBState, cap_chunk, true), + DEFINE_PROP_BOOL("x-allow-chunk-migration", CRBState, + allow_chunk_migration, true), }; static void tpm_crb_reset(void *dev) @@ -422,6 +478,7 @@ static void tpm_crb_reset(void *dev) static void tpm_crb_realize(DeviceState *dev, Error **errp) { CRBState *s = CRB(dev); + int ret; if (!tpm_find()) { error_setg(errp, "at most one TPM device is permitted"); @@ -431,6 +488,15 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp) error_setg(errp, "'tpmdev' property is required"); return; } + if (s->cap_chunk && !s->allow_chunk_migration) { + error_setg(&s->migration_blocker, + "The tpm-crb device does not support chunk migration with " + "machine version less than 11.1"); + ret = migrate_add_blocker_normal(&s->migration_blocker, errp); + if (ret < 0) { + return; + } + } memory_region_init_io(&s->mmio, OBJECT(s), &tpm_crb_memory_ops, s, "tpm-crb-mmio", sizeof(s->regs)); @@ -463,6 +529,11 @@ static void tpm_crb_unrealize(DeviceState *dev) g_clear_pointer(&s->command_buffer, g_byte_array_unref); g_clear_pointer(&s->response_buffer, g_byte_array_unref); + + if (s->migration_blocker) { + migrate_del_blocker(&s->migration_blocker); + error_free(s->migration_blocker); + } } static void tpm_crb_class_init(ObjectClass *klass, const void *data)Regards, Arun MenonRegards, Arun Menon
