* Re: [PATCH] vhost/migration: Fix incorrect size used in inflight->addr in VMSD
2026-07-28 15:39 [PATCH] vhost/migration: Fix incorrect size used in inflight->addr in VMSD Peter Xu
@ 2026-07-28 20:57 ` Fabiano Rosas
2026-07-29 14:00 ` Peter Xu
2026-07-28 23:17 ` Michael S. Tsirkin
2026-07-29 7:25 ` Alexandr Moshkov
2 siblings, 1 reply; 15+ messages in thread
From: Fabiano Rosas @ 2026-07-28 20:57 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: peterx, Michael S. Tsirkin, Stefano Garzarella,
김승중, Alexandr Moshkov
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.
We could even lift that check up into vmstate_pre_load and reject
there globally, don't even touch the virtio code.
/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.
> -
> #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()
> }
> };
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] vhost/migration: Fix incorrect size used in inflight->addr in VMSD
2026-07-28 20:57 ` Fabiano Rosas
@ 2026-07-29 14:00 ` Peter Xu
2026-07-29 14:25 ` Michael S. Tsirkin
2026-07-29 14:27 ` Fabiano Rosas
0 siblings, 2 replies; 15+ messages in thread
From: Peter Xu @ 2026-07-29 14:00 UTC (permalink / raw)
To: Fabiano Rosas
Cc: qemu-devel, Michael S. Tsirkin, Stefano Garzarella,
김승중, Alexandr Moshkov
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] vhost/migration: Fix incorrect size used in inflight->addr in VMSD
2026-07-29 14:00 ` Peter Xu
@ 2026-07-29 14:25 ` Michael S. Tsirkin
2026-07-29 14:27 ` Fabiano Rosas
1 sibling, 0 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2026-07-29 14:25 UTC (permalink / raw)
To: Peter Xu
Cc: Fabiano Rosas, qemu-devel, Stefano Garzarella,
김승중, Alexandr Moshkov
On Wed, Jul 29, 2026 at 10:00:13AM -0400, Peter Xu wrote:
> 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.
It's not that size needs to be >2G. But it might be nice for some code
to keep it as u64 for its own reasons to avoid issues like overruns
during math ops.
>
> 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,
The issue is that 2G is not a sane limit either.
So what we really want is a macro that supplies
a size limit. That would be an improvement.
> >
> > > -
> > > #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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] vhost/migration: Fix incorrect size used in inflight->addr in VMSD
2026-07-29 14:00 ` Peter Xu
2026-07-29 14:25 ` Michael S. Tsirkin
@ 2026-07-29 14:27 ` Fabiano Rosas
2026-07-29 15:44 ` Peter Xu
1 sibling, 1 reply; 15+ messages in thread
From: Fabiano Rosas @ 2026-07-29 14:27 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Michael S. Tsirkin, Stefano Garzarella,
김승중, Alexandr Moshkov
Peter Xu <peterx@redhat.com> writes:
> 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?
>
I was thinking of fixing this on the vmstate size and not changing the
virtio code. But yes, it's better to fix properly.
>>
>> 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.
>
Hm, but isn't it better to have a global cap anyway? So we don't need
every device code to change with similar checks.
> 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.
>
I agree with making all u64. I think we can actually remove all the
extra VMS_VARRAY_* and VBUFFER_* flags and instead doa single type-check
of "int <= 64bit". Give me a couple of hours and I will post an RFC.
> 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()
>> > }
>> > };
>>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] vhost/migration: Fix incorrect size used in inflight->addr in VMSD
2026-07-29 14:27 ` Fabiano Rosas
@ 2026-07-29 15:44 ` Peter Xu
2026-07-29 16:13 ` Fabiano Rosas
0 siblings, 1 reply; 15+ messages in thread
From: Peter Xu @ 2026-07-29 15:44 UTC (permalink / raw)
To: Fabiano Rosas
Cc: qemu-devel, Michael S. Tsirkin, Stefano Garzarella,
김승중, Alexandr Moshkov
On Wed, Jul 29, 2026 at 11:27:44AM -0300, Fabiano Rosas wrote:
> Hm, but isn't it better to have a global cap anyway? So we don't need
> every device code to change with similar checks.
>
> > 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.
> >
>
> I agree with making all u64. I think we can actually remove all the
> extra VMS_VARRAY_* and VBUFFER_* flags and instead doa single type-check
> of "int <= 64bit". Give me a couple of hours and I will post an RFC.
I confess I don't yet get what is the global cap you mentioned.. but sure,
I'll wait and read the RFC first.
--
Peter Xu
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] vhost/migration: Fix incorrect size used in inflight->addr in VMSD
2026-07-29 15:44 ` Peter Xu
@ 2026-07-29 16:13 ` Fabiano Rosas
2026-07-29 17:40 ` Peter Xu
2026-07-29 17:48 ` Peter Xu
0 siblings, 2 replies; 15+ messages in thread
From: Fabiano Rosas @ 2026-07-29 16:13 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Michael S. Tsirkin, Stefano Garzarella,
김승중, Alexandr Moshkov
Peter Xu <peterx@redhat.com> writes:
> On Wed, Jul 29, 2026 at 11:27:44AM -0300, Fabiano Rosas wrote:
>> Hm, but isn't it better to have a global cap anyway? So we don't need
>> every device code to change with similar checks.
>>
>> > 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.
>> >
>>
>> I agree with making all u64. I think we can actually remove all the
>> extra VMS_VARRAY_* and VBUFFER_* flags and instead doa single type-check
>> of "int <= 64bit". Give me a couple of hours and I will post an RFC.
>
> I confess I don't yet get what is the global cap you mentioned.. but sure,
> I'll wait and read the RFC first.
I was thinking the vmstate code could define a limit to the size (of
anything) and always enforce it. The device code can then use some
custom macros (not yet existent) to limit even further.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] vhost/migration: Fix incorrect size used in inflight->addr in VMSD
2026-07-29 16:13 ` Fabiano Rosas
@ 2026-07-29 17:40 ` Peter Xu
2026-07-29 17:48 ` Peter Xu
1 sibling, 0 replies; 15+ messages in thread
From: Peter Xu @ 2026-07-29 17:40 UTC (permalink / raw)
To: Fabiano Rosas
Cc: qemu-devel, Michael S. Tsirkin, Stefano Garzarella,
김승중, Alexandr Moshkov
On Wed, Jul 29, 2026 at 01:13:27PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
>
> > On Wed, Jul 29, 2026 at 11:27:44AM -0300, Fabiano Rosas wrote:
> >> Hm, but isn't it better to have a global cap anyway? So we don't need
> >> every device code to change with similar checks.
> >>
> >> > 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.
> >> >
> >>
> >> I agree with making all u64. I think we can actually remove all the
> >> extra VMS_VARRAY_* and VBUFFER_* flags and instead doa single type-check
> >> of "int <= 64bit". Give me a couple of hours and I will post an RFC.
> >
> > I confess I don't yet get what is the global cap you mentioned.. but sure,
> > I'll wait and read the RFC first.
>
> I was thinking the vmstate code could define a limit to the size (of
> anything) and always enforce it. The device code can then use some
> custom macros (not yet existent) to limit even further.
Ah I see.
With 32bit limit (let's assume we don't have that further "it's int32_t"
problem..), it's already throttled to 32bit max. I added it into vhost
code only because in reality it was a 64bit internally. None of the rest
users should need it since they should be sticking with 32bit fields.
--
Peter Xu
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] vhost/migration: Fix incorrect size used in inflight->addr in VMSD
2026-07-29 16:13 ` Fabiano Rosas
2026-07-29 17:40 ` Peter Xu
@ 2026-07-29 17:48 ` Peter Xu
2026-07-29 18:57 ` Michael S. Tsirkin
1 sibling, 1 reply; 15+ messages in thread
From: Peter Xu @ 2026-07-29 17:48 UTC (permalink / raw)
To: Fabiano Rosas
Cc: qemu-devel, Michael S. Tsirkin, Stefano Garzarella,
김승중, Alexandr Moshkov
On Wed, Jul 29, 2026 at 01:13:27PM -0300, Fabiano Rosas wrote:
> I was thinking the vmstate code could define a limit to the size (of
> anything) and always enforce it. The device code can then use some
> custom macros (not yet existent) to limit even further.
PS: one more thing to mention on "further limit":
To me, that was not a real thing that will help anything for real users.
My guess is, it will only be something we can use to "convince" people stop
opening security tickets.. and say "hey, see, we have an upper bound".
IMHO no limit is really fine for QEMU's complex use case, it's because QEMU
is a complex userapp, if it's a daemon and more accessible we may need to
be more careful, but it's not: it's a hypervisor, here migration protocol
(as one of the network consumer) is very dedicated, perf / maintenance /
.. matter more than that upper limit. There're other things that might be
more risky, like if we have NBD server listening and it's more of an attack
surface. Even if so, I still think a VM userapp is very special.
If I design migration from scratch, I would allow some migration command to
dynamically create ramblocks, for example, and obviously that can really be
any size: limiting it to 4T will stop working when one wants to create a
12T VM. So to me, it's much simpler we say migration stream must be
trusted, and I expect dest QEMU can allocate any buffer it needs, until it
eats the whole system memory. I really don't see much real risk..
That's partly why personally I want us to spend less time on all these
limits.. because then we only handle trusted data stream, we can keep in
mind whenever we want to be safer, but it's not 100% required and it's
different from adhoc network servers. When the protocol allows "arbitrary
size" I think we should just let it happen, and IMHO it avoids unnecessary
work.
--
Peter Xu
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] vhost/migration: Fix incorrect size used in inflight->addr in VMSD
2026-07-29 17:48 ` Peter Xu
@ 2026-07-29 18:57 ` Michael S. Tsirkin
2026-07-29 19:27 ` Peter Xu
0 siblings, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2026-07-29 18:57 UTC (permalink / raw)
To: Peter Xu
Cc: Fabiano Rosas, qemu-devel, Stefano Garzarella,
김승중, Alexandr Moshkov
On Wed, Jul 29, 2026 at 01:48:25PM -0400, Peter Xu wrote:
> So to me, it's much simpler we say migration stream must be
> trusted, and I expect dest QEMU can allocate any buffer it needs, until it
> eats the whole system memory. I really don't see much real risk..
It's not risk due to migration, specifically. But making qemu
drink up terabytes from the guest would be problematic, right?
Putting qemu in a cgroup with restricted total memory
would be one way to prevent this class of security issue,
and a robust one.
But that, in turn, is impossible if qemu insists on allocating
unlimited memory at the drop of a hat.
However, migration is hardly the worst offender here, and I am
not at all sure we need to start with it if we even want
to address this limitation.
--
MST
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] vhost/migration: Fix incorrect size used in inflight->addr in VMSD
2026-07-29 18:57 ` Michael S. Tsirkin
@ 2026-07-29 19:27 ` Peter Xu
0 siblings, 0 replies; 15+ messages in thread
From: Peter Xu @ 2026-07-29 19:27 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Fabiano Rosas, qemu-devel, Stefano Garzarella,
김승중, Alexandr Moshkov
On Wed, Jul 29, 2026 at 02:57:57PM -0400, Michael S. Tsirkin wrote:
> On Wed, Jul 29, 2026 at 01:48:25PM -0400, Peter Xu wrote:
> > So to me, it's much simpler we say migration stream must be
> > trusted, and I expect dest QEMU can allocate any buffer it needs, until it
> > eats the whole system memory. I really don't see much real risk..
>
> It's not risk due to migration, specifically. But making qemu
> drink up terabytes from the guest would be problematic, right?
>
> Putting qemu in a cgroup with restricted total memory
> would be one way to prevent this class of security issue,
> and a robust one.
>
> But that, in turn, is impossible if qemu insists on allocating
> unlimited memory at the drop of a hat.
Just to clarify at least one thing.. we have two attack surfaces here and
they're very different IMHO:
(1) guest behavior caused memory allocation, or,
(2) migration stream caused memory allocation.
AFAIU, (1) is more severe. All my points only apply to (2).
>
> However, migration is hardly the worst offender here, and I am
> not at all sure we need to start with it if we even want
> to address this limitation.
Agree.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] vhost/migration: Fix incorrect size used in inflight->addr in VMSD
2026-07-28 15:39 [PATCH] vhost/migration: Fix incorrect size used in inflight->addr in VMSD Peter Xu
2026-07-28 20:57 ` Fabiano Rosas
@ 2026-07-28 23:17 ` Michael S. Tsirkin
2026-07-29 7:25 ` Alexandr Moshkov
2 siblings, 0 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2026-07-28 23:17 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Fabiano Rosas, Stefano Garzarella,
김승중, Alexandr Moshkov
On Tue, Jul 28, 2026 at 11:39:42AM -0400, 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: 김승중 <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.
I agree.
> ---
> 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()
> }
> };
> --
> 2.54.0
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] vhost/migration: Fix incorrect size used in inflight->addr in VMSD
2026-07-28 15:39 [PATCH] vhost/migration: Fix incorrect size used in inflight->addr in VMSD Peter Xu
2026-07-28 20:57 ` Fabiano Rosas
2026-07-28 23:17 ` Michael S. Tsirkin
@ 2026-07-29 7:25 ` Alexandr Moshkov
2026-07-29 8:49 ` Michael S. Tsirkin
2 siblings, 1 reply; 15+ messages in thread
From: Alexandr Moshkov @ 2026-07-29 7:25 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Michael S. Tsirkin, Fabiano Rosas, Stefano Garzarella,
김승중
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: 김승중 <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.
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/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), \
> -}
> -
> #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()
> }
> };
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] vhost/migration: Fix incorrect size used in inflight->addr in VMSD
2026-07-29 7:25 ` Alexandr Moshkov
@ 2026-07-29 8:49 ` Michael S. Tsirkin
2026-07-29 14:16 ` Peter Xu
0 siblings, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2026-07-29 8:49 UTC (permalink / raw)
To: Alexandr Moshkov
Cc: Peter Xu, qemu-devel, Fabiano Rosas, Stefano Garzarella,
김승중
On Wed, Jul 29, 2026 at 12:25:05PM +0500, Alexandr Moshkov wrote:
>
> 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: 김승중 <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.
>
> 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;
> }
At which point I ask whether open coding all this mess
is so much better.
Way I look at it, vmstate machinery had an API of storing size in u64
that it failed to implement correctly. Why not fix it?
> >
> > 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), \
> > -}
> > -
> > #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()
> > }
> > };
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] vhost/migration: Fix incorrect size used in inflight->addr in VMSD
2026-07-29 8:49 ` Michael S. Tsirkin
@ 2026-07-29 14:16 ` Peter Xu
0 siblings, 0 replies; 15+ messages in thread
From: Peter Xu @ 2026-07-29 14:16 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alexandr Moshkov, qemu-devel, Fabiano Rosas, Stefano Garzarella,
김승중
On Wed, Jul 29, 2026 at 04:49:13AM -0400, Michael S. Tsirkin wrote:
> On Wed, Jul 29, 2026 at 12:25:05PM +0500, Alexandr Moshkov wrote:
> >
> > 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: 김승중 <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.
> >
> > 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;
> > }
Thanks for the testing and report, Alexandr. Obviously I only kept in mind
of the cross-binary case.. I'll see if I'll respin with the fix or
something different.
>
> At which point I ask whether open coding all this mess
> is so much better.
>
>
> Way I look at it, vmstate machinery had an API of storing size in u64
> that it failed to implement correctly. Why not fix it?
I explained in my reply to Fabiano:
https://lore.kernel.org/qemu-devel/amoHbVsgzkYclCt2@x1.local/
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 15+ messages in thread