Re: [PATCH] vhost/migration: Fix incorrect size used in inflight->addr in VMSD

On 7/28/26 20:39, Peter Xu wrote:
> It was overlooked that VMSTATE_VBUFFER_UINT64() won't really work with an
> uint64_t, as vmstate core only treats the size as 32bits, and maximum
> INT32_MAX (see vmstate_size()).
>
> Considering that we do not need real 64bits for the size, stick with the 2G
> limit, converting the size field into 32bits.
>
> Since we can't touch the wire protocol on migration from an old QEMU, we
> can't directly modify the type of size to uint32_t.  Instead, we need to
> introduce a temporary variable for this extremely rare issue __size_32bits
> to be used only for VMSTATE_VBUFFER_UINT32().  Document it and name it
> weird enough so people won't get confused on having two size variables.
>
> Remove VMSTATE_VBUFFER_UINT64() altogether, because it was never going to
> be used right.  It means QEMU will only support 2G max for VMS_VBUFFER.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3675
> Reported-by: 김승중 <[email protected]>
> Cc: Alexandr Moshkov <[email protected]>
> Cc: Michael S. Tsirkin <[email protected]>
> Cc: Fabiano Rosas <[email protected]>
> Fixes: 3a80ff0721 ("vhost: add vmstate for inflight region with inner buffer")
> Signed-off-by: Peter Xu <[email protected]>
> ---
>
> PS1: I only did smoke test as I'm not fluent with vhost inflight feature.
> Please kindly try it out if possible. In general, migrations from older
> QEMU should work even after applied.  One can also treat this as partly-RFC
> from that.

It looks like inflight migration is broken:

qemu-system-x86_64: Missing section footer for 
0000:00:02.0:00.0:00.0/vhost-user-blk
migrate_error error=load of migration failed: Invalid argument: Section 
footer error, section_id: 53
qemu-system-x86_64: load of migration failed: Invalid argument: Section 
footer error, section_id: 53

As far as I understand this happen because __size_32bits not initialized 
on source - it's never set. So vmstate_size() reads 0, and zero-lenght 
buffer is written into migration stream.
The destination then allocates the correct buffer but reads 0 bytes from 
the stream, leaving unconsumed data and causing the section footer check 
to fail.

It can be fixed with adding pre_save (or pre_save_errp) to 
vmstate_vhost_inflight_region_buffer that initialize __size_32bits from 
size:

static int vhost_inflight_buffer_pre_save(void *opaque)
{
     struct vhost_inflight *inflight = opaque;
     /* Only used in VMSTATE_VBUFFER_UINT32() */
     inflight->__size_32bits = inflight->size;
     return 0;
}

>
> PS2: Michael, we have just discussed what we should define as CVE for
> migration, and this one shouldn't fall into CVE category, please refer to:
>
> https://lore.kernel.org/r/[email protected]
>
> So I didn't yet attach CVE tag.  Please correct if I'm wrong, thanks.
> ---
>   include/hw/virtio/vhost.h   |  5 +++++
>   include/migration/vmstate.h | 10 ----------
>   hw/virtio/vhost.c           | 19 ++++++++++++++-----
>   3 files changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 684bafcaad..1d1cc24c04 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -17,6 +17,11 @@ struct vhost_inflight {
>       int fd;
>       void *addr;
>       uint64_t size;
> +    /*
> +     * This is a temporary variable only used during migration loading to
> +     * satisfy VMSTATE_VBUFFER_UINT32() typing. Please use @size otherwise.
> +     */
> +    uint32_t __size_32bits;
>       uint64_t offset;
>       uint16_t queue_size;
>   };
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 1b7f295417..a349b2d84a 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -782,16 +782,6 @@ extern const VMStateInfo vmstate_info_g_byte_array;
>       .offset       = offsetof(_state, _field),                        \
>   }
>   
> -#define VMSTATE_VBUFFER_UINT64(_field, _state, _version, _test, _field_size) { \
> -    .name         = (stringify(_field)),                             \
> -    .version_id   = (_version),                                      \
> -    .field_exists = (_test),                                         \
> -    .size_offset  = vmstate_offset_value(_state, _field_size, uint64_t),\
> -    .info         = &vmstate_info_buffer,                            \
> -    .flags        = VMS_VBUFFER | VMS_POINTER,                       \
> -    .offset       = offsetof(_state, _field),                        \
> -}
> -
>   #define VMSTATE_VBUFFER_ALLOC_UINT32(_field, _state, _version,       \
>                                        _test, _field_size) {           \
>       .name         = (stringify(_field)),                             \
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index af41841b52..e7c570d0f5 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -2022,15 +2022,24 @@ void vhost_get_features_ex(struct vhost_dev *hdev,
>   static bool vhost_inflight_buffer_pre_load(void *opaque, Error **errp)
>   {
>       struct vhost_inflight *inflight = opaque;
> -
>       int fd = -1;
> -    void *addr = qemu_memfd_alloc("vhost-inflight", inflight->size,
> -                                  F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL,
> -                                  &fd, errp);
> +    void *addr;
> +
> +    if (inflight->size > INT32_MAX) {
> +        error_setg(errp, "inflight size '%"PRIu64"' exceeds "
> +                   "migration limit '%"PRIu32"'", inflight->size, INT32_MAX);
> +        return false;
> +    }
> +
> +    addr = qemu_memfd_alloc("vhost-inflight", inflight->size,
> +                            F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL,
> +                            &fd, errp);
>       if (!addr) {
>           return false;
>       }
>   
> +    /* Only used in VMSTATE_VBUFFER_UINT32() */
> +    inflight->__size_32bits = inflight->size;
>       inflight->offset = 0;
>       inflight->addr = addr;
>       inflight->fd = fd;
> @@ -2042,7 +2051,7 @@ const VMStateDescription vmstate_vhost_inflight_region_buffer = {
>       .name = "vhost-inflight-region/buffer",
>       .pre_load_errp = vhost_inflight_buffer_pre_load,
>       .fields = (const VMStateField[]) {
> -        VMSTATE_VBUFFER_UINT64(addr, struct vhost_inflight, 0, NULL, size),
> +        VMSTATE_VBUFFER_UINT32(addr, struct vhost_inflight, 0, NULL, __size_32bits),
>           VMSTATE_END_OF_LIST()
>       }
>   };