Re: [PULL 17/30] vmstate: fix type confusion in vmstate_size() for VMSTATE_VBUFFER_UINT64

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jul 27, 2026 at 08:49:44AM -0400, Peter Xu wrote:
> On Mon, Jul 27, 2026 at 04:53:15AM -0400, Michael S. Tsirkin wrote:
> > On Sun, Jul 26, 2026 at 08:31:57PM -0400, Peter Xu wrote:
> > > This patch is migration only change and hasn't been reviewed.  Give us a few
> > > days to review it?
> > > Can't do it now because it is on cellphone and I just bathed my son.
> > > 
> > > Please hold off merging.
> > > 
> > > Thanks.
> > 
> > OK.  I'll drop this and you will merge it through the migration tree
> > as appropriate?
> 
> Let's drop it first so it won't block this pull.  I can definitely pick it
> up when ready, but I want to discuss on how to fix first.  Which tree to
> pick it up isn't a huge matter to me, but the review.

Done.
Pls include:

Reported-by: Seungjung Kim <seungjung0711@gmail.com>

when you are merging this.

> We found a bunch of randomly introduced VMSTATE flags in the past, I used
> to remove some, at least my plan is in the future each time we introduce a
> new one (by request from other modules) we better justify it before landing
> too easily, in case it needs a revert again.
> 
> This vhost regression started 11.0 so IIUC we don't need to rush in 11.1.
> I just noticed I read that patch and didn't notice this, my bad to not have
> noticed..
> 
> For this one specifically..
> 
> > 
> > > On Sun, Jul 26, 2026, 5:29 p.m. Michael S. Tsirkin <mst@redhat.com> wrote:
> > > 
> > >     vhost user currently saves the inflight buffer to the migration stream
> > >     using VMSTATE_VBUFFER_UINT64. The size is controlled by the vhost-user
> > >     backend.
> > > 
> > >     But the implementation of that is broken if size is >2G: it stores the
> > >     buffer size in a uint64_t field, but vmstate_size() always reads the
> > >     size field as int32_t regardless of the macro used. This, in turn,
> > >     causes negative or truncated lengths on load, leading to undersized
> > >     allocations and down the road out-of-bounds buffer access.
> > > 
> > >     There's no practical reason to support such large sizes, so it's enough
> 
> If there's no real demand to use 2G or more, I suggest we change size to
> int32_t instead on the user, and revert f6fdd8b2 VMSTATE_VBUFFER_UINT64().
> 
> Fabiano and I were looking at rest 20+ possible security related bugs in
> the past 1-2 weeks, I've some patches to be posted for 11.2 too when we
> went through all of them. One relevant patch I haven't sent but will do
> soon:
> 
> https://gitlab.com/peterx/qemu/-/commit/afd2cb25defdf78a4ab5279cc16634884792c57a
> 
> We encountered quite a few of possible over-allocation on destionation side
> by manipulating on-wire length field like this one, I believe Fabiano is
> looking at how to further limit that from 2G if ever possible per-user.
> 
> I'm not sure if that idea will fly, but I think anything bigger than 2G
> definitely is not suggested for now when it's only for a type match not
> real demand..
> 
> I believe VMSTATE_VBUFFER_UINT32 is problematic on its own too, I'll see
> how to fix that.  That one is slightly easier, worst case is we bail out
> for 2G (hence ignore bit 31, which shouldn't be used in reality for legit
> users), but I'll think about it.
> 
> Thanks,
> 
> > >     to validate: read the field as uint64_t when VMS_VBUFFER_UINT64 is set,
> > >     reject negative or oversized values, and propagate errors to
> > >     vmstate_load_vmsd().
> > > 
> > >     Note: the large value is coming from the backend, not guest, so this
> > >     shouldn't be considered a security issue. The CVE was assigned before
> > >     the qemu security policy was updated to exclude this class of bugs.
> > > 
> > >     Fixes: CVE-2026-6426
> > >     Fixes: f6fdd8b2bd ("vmstate: introduce VMSTATE_VBUFFER_UINT64")
> > >     Cc: Alexandr Moshkov <dtalexundeer@yandex-team.ru>
> > >     Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3675
> > >     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >     Message-ID: <
> > >     801b1501ee10241f7ac49a10570d548352b36347.1784890517.git.mst@redhat.com>
> > >     ---
> > >      include/migration/vmstate.h |  5 ++++-
> > >      migration/vmstate.c         | 31 ++++++++++++++++++++++++++++---
> > >      2 files changed, 32 insertions(+), 4 deletions(-)
> > > 
> > >     diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > >     index 1b7f295417..e7095cd977 100644
> > >     --- a/include/migration/vmstate.h
> > >     +++ b/include/migration/vmstate.h
> > >     @@ -168,6 +168,9 @@ enum VMStateFlags {
> > >           */
> > >          VMS_ARRAY_OF_POINTER_AUTO_ALLOC = 0x10000,
> > > 
> > >     +    /* Use a uint64_t size field for VMS_VBUFFER instead of int32_t. */
> > >     +    VMS_VBUFFER_UINT64              = 0x40000,
> > >     +
> > >          /* Marker for end of list */
> > >          VMS_END                         = 0x20000,
> > >      };
> > >     @@ -788,7 +791,7 @@ extern const VMStateInfo vmstate_info_g_byte_array;
> > >          .field_exists = (_test),                                         \
> > >          .size_offset  = vmstate_offset_value(_state, _field_size, uint64_t),\
> > >          .info         = &vmstate_info_buffer,                            \
> > >     -    .flags        = VMS_VBUFFER | VMS_POINTER,                       \
> > >     +    .flags        = VMS_VBUFFER | VMS_VBUFFER_UINT64 | VMS_POINTER,  \
> > >          .offset       = offsetof(_state, _field),                        \
> > >      }
> > > 
> > >     diff --git a/migration/vmstate.c b/migration/vmstate.c
> > >     index 50ebe37845..d7f03a9f5b 100644
> > >     --- a/migration/vmstate.c
> > >     +++ b/migration/vmstate.c
> > >     @@ -103,10 +103,24 @@ static int vmstate_size(void *opaque, const
> > >     VMStateField *field)
> > >          int size;
> > > 
> > >          if (field->flags & VMS_VBUFFER) {
> > >     -        size = *(int32_t *)(opaque + field->size_offset);
> > >     -        if (field->flags & VMS_MULTIPLY) {
> > >     -            size *= field->size;
> > >     +        uint64_t usize64;
> > >     +
> > >     +        if (field->flags & VMS_VBUFFER_UINT64) {
> > >     +            usize64 = *(uint64_t *)(opaque + field->size_offset);
> > >     +        } else {
> > >     +            int32_t ssize32 = *(int32_t *)(opaque + field->size_offset);
> > >     +            if (ssize32 < 0) {
> > >     +                return -1;
> > >     +            }
> > >     +            usize64 = ssize32;
> > >              }
> > >     +        if (field->flags & VMS_MULTIPLY) {
> > >     +            usize64 *= field->size;
> > >     +        }
> > >     +        if (usize64 > INT_MAX) {
> > >     +            return -1;
> > >     +        }
> > >     +        size = usize64;
> > >          } else if (field->flags & VMS_ARRAY_OF_POINTER) {
> > >              /*
> > >               * For an array of pointer, the each element is always size of a
> > >     @@ -337,6 +351,11 @@ bool vmstate_load_vmsd(QEMUFile *f, const
> > >     VMStateDescription *vmsd,
> > >                  void *first_elem = opaque + field->offset;
> > >                  int i, n_elems = vmstate_n_elems(opaque, field);
> > >                  int size = vmstate_size(opaque, field);
> > >     +            if (size < 0) {
> > >     +                error_setg(errp, "VMState field '%s': invalid size",
> > >     +                           field->name);
> > >     +                return false;
> > >     +            }
> > > 
> > >                  vmstate_handle_alloc(first_elem, field, opaque);
> > >                  if (field->flags & VMS_POINTER) {
> > >     @@ -661,6 +680,12 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const
> > >     VMStateDescription *vmsd,
> > >                  bool use_dynamic_array =
> > >                      field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
> > > 
> > >     +            if (size < 0) {
> > >     +                error_setg(errp, "VMState field '%s': invalid size",
> > >     +                           field->name);
> > >     +                ok = false;
> > >     +                goto out;
> > >     +            }
> > >                  trace_vmstate_save_state_loop(vmsd->name, field->name,
> > >     n_elems);
> > >                  if (field->flags & VMS_POINTER) {
> > >                      first_elem = *(void **)first_elem;
> > >     --
> > >     MST
> > > 
> > > 
> > 
> 
> -- 
> Peter Xu