Re: [PATCH v5 06/10] hw/tpm: Add support for VM migration with TPM CRB chunking

[Arun Menon <armenon@redhat.com>]

On Mon, May 04, 2026 at 10:33:38AM -0400, Stefan Berger wrote:
> 
> 
> 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 <armenon@redhat.com>
> > > > > > 
> > > > > > - 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 <armenon@redhat.com>
> > > > > > ---
> > > > > >     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.

Sure. I have updated it in v6. Thank you.

> 
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > > +    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 Menon
> > > > 
> > > 
> > 
> > Regards,
> > Arun Menon
> > 
> 

Regards,
Arun Menon