"Michael S. Tsirkin" <[email protected]> writes:
> On Mon, Jul 27, 2026 at 04:29:55PM -0400, Peter Xu wrote:
>> On Mon, Jul 27, 2026 at 03:06:38PM -0400, Michael S. Tsirkin wrote:
>> > 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.
>> > >
>> > > 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 <[email protected]>
>> > > > > 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().
>> >
>> > We can't just change it we'll need some tricks to avoid breaking existing
>> > migration format. I don't see how it will be much simpler if you take
>> > that into account.
>>
>> It's not part of the stream, right?
>
> I think it is?
>
> VMSTATE_UINT64(size, struct vhost_inflight) at hw/virtio/vhost.c
> eventually leads to qemu_put_be64s / qemu_get_be64s in
> migration/vmstate-types.c
>
The point is that this "size" is already included on the stream by
itself, changing the VMSTATE_VBUFFER_UINT64 to int32 doesn't affect
it.
There's a type_check that will complain during save that
type(inflight->size) != INT32 (from the macro), but we can change that.
#define vmstate_offset_value(_state, _field, _type) \
(offsetof(_state, _field) + \
type_check(_type, typeof_field(_state, _field)))
>> Here IIUC the VMSD field "size" only hard-coded the binary offset to fetch
>> the int32_t from the object* pointer. IIUC we can simply change it to
>> int32_t and IIUC it will be able to recv stream from src with uint64_t.
>> The stream shouldn't be affected when e.g. we migrate from an older QEMU.
>
>
> I don't really know what you mean. Yes vmsd macros tie migration stream
> and the storage format. it kinda makes it easy to add one but the
> cost is issues like this.
>
>
>
>
>> >
>> > > 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..
>> >
>> > Frankly 2G is crazy too. I see little reason to focus specifically on >2G.
>>
>> Yeah..
>>
>> >
>> >
>> > > 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,
>> >
>> > I just do not see (almost) any case where migration destination
>> > does not know the actual size needed, or at least
>> > an upper bound on such.
>>
>> In many cases we transfer the length first then the array following that
>> with the length. That's a common trick we played like vhost here.
>
> we need to start specifying the cap for these.
>
I agree. Most things will have a sane cap once you think hard enough. Of
course, there's a risk of selecting a limit too low, but currently we
can't even do that.
I think the first step is defining new macros so that every len/size/etc
gets properly identified. The VMSTATE_VBUFFER_* are already a start, we
can add an aditional member that doesn't go on the stream and check
against it when loading.
> The upper bound is indeed a challenge, I confess I don't know an answer,
> and we will need to be careful on adding anything that might break a legit
> user. I believe Fabiano might have some better clue there, so I'd leave it
> to him until I can read his patches.
>
I'm also trying to look ahead into how we'll apply whatever fix we come
up with here to the pile of unstructured qemu_put|get_byte we have all
over the place. I'm experimenting with how much code we can convert into
VMStateDescription/VMStateField. The QEMU_VM_* flags themselves seem to
be easy to do (then we version the vmstate, deprecate, etc).
>> 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 <[email protected]>
>> > > > > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3675
>> > > > > Signed-off-by: Michael S. Tsirkin <[email protected]>
>> > > > > Message-ID: <
>> > > > >
>> > > > > 801b1501ee10241f7ac49a10570d548352b36347.1784890517.git....@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
>> >
>>
>> --
>> Peter Xu
