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

[Peter Xu <peterx@redhat.com>]

On Tue, Jul 28, 2026 at 05:57:56PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > 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: 김승중 <seungjung0711@gmail.com>
> > Cc: Alexandr Moshkov <dtalexundeer@yandex-team.ru>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Fabiano Rosas <farosas@suse.de>
> > Fixes: 3a80ff0721 ("vhost: add vmstate for inflight region with inner buffer")
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >
> > 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.
> >
> > 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/20260721131457.3062767-1-farosas@suse.de
> >
> > 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),                        \
> > -}
> 
> Why don't you just leave this macro around, put a "do not use" comment
> above it and remove the type check for this one instance? You're already
> checking against INT32_MAX during pre_load.

What's the benefit of keeping this global macro if we only allow one user
to use it?

Commenting to say "don't use" is not guaranteeing anything, at least we
should also rename the macro to some weird name to make people notice.
Keeping the macro name like this OTOH suggests abuse.  But if we think it
should only be used in 1 place, fixing the one place might be better?

> 
> We could even lift that check up into vmstate_pre_load and reject
> there globally, don't even touch the virtio code.

Personally, I don't like the idea leaking VBUFFER impl details into
vmstate_pre_load() which is so far only a wrapper, especially only for this
one instance.. which we do not suggest future users to use.

If we go this route, I'd rather merge Michael's version to support u64,
even if we don't need a u64 size.  But I really don't want to introduce yet
another VMS flag just for this... we'll have no real use if we have noticed
this problem when the vhost inflight patch was reviewed.  It will be a
uint32_t or int32_t already.  I just can't come up with some users need
size >2G.

The recent AI reports just make such feeling stronger: we just used the
reason "max 2G, should be fine" when AI reports that unlimited size
allocation problem, now we will need to wait for another AI report which
says "it's not 2G anymore, 1<<64-1 that is", if we don't come up with a
proper limit for VMSD field allocations.

> 
> 
> /rant
> ... what's even the point of having per-type variants of VMSTATE macros
> that exist just to type-check the extra offsets (.num_offset,
> .size_offset as opposed to .offset)?
> 
> For instance, look at vmstate_n_elems casting opaque data to 32 and
> sub-32bit size and assigning to an int! What do we gain from that? It's
> circular reasoning that does nothing aside from bothering the person
> writing the vmstate.
> 
> Right? Maybe I'm missing something, it's the end of the day already.

I can also only guess, which is.. I believe Michael was right, the initial
vmstate code was simply broken where it should have considered the type of
size fields of all kinds, but forgot, and it just worked because nobody
needed u64 for a size field.

Said that, I still want to see if we can avoid introducing that at all.  To
me, I still prefer this patch (after fixing pre_save()..).  But let me know
if I didn't convince any of you.. we can discuss.

Thanks,

> 
> > -
> >  #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()
> >      }
> >  };
> 

-- 
Peter Xu