On Mon, Jul 27, 2026 at 09:17:25AM -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. > > Done. > Pls include: > > Reported-by: Seungjung Kim <[email protected]> > > when you are merging this.
Michael, Please continue read at least until [1] below. I want to discuss if we should fix it in another way.. Thanks, > > > 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(). [1] > > > > 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 <[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
