Re: [PATCH 0/4] migration: Remove extra type-checking from vmstate macros

Peter Xu <[email protected]> writes:

> On Thu, Jul 30, 2026 at 10:09:30AM -0400, Peter Xu wrote:
>> On Wed, Jul 29, 2026 at 07:52:23PM -0300, Fabiano Rosas wrote:
>> > Hi, this is basically what I ranted about in:
>> > https://lore.kernel.org/r/[email protected]
>> > 
>> > I'm replacing the per-integer-size type checks with a single "int that
>> > fits in 32bit" check. This allows several lines of duplicated code to
>> > be removed.
>> > 
>> > I haven't changed the macro names in the device code yet. If this
>> > series gets positive feedback then I'll send per-subsystem patches
>> > doing that.
>> > 
>> > CI run: https://gitlab.com/farosas/qemu/-/pipelines/2716814081
>> > Also tested:
>> > - migration-test --full --thorough
>> > - x86_64 compat run forwards and backwards for previous 3 QEMU releases
>> > - s390x compat run forwards and backwards for previous 2 QEMU releases
>> > - ppc64 compat run forwards and backwards for previous QEMU release
>> > - migration-test smoke ASAN/UBSAN run
>> > 
>> > Fabiano Rosas (4):
>> >   migration: Remove VMSTATE_ARRAY_INT32_UNSAFE
>> >   migration: Introduce VMStateOffset
>> >   migration: Remove redundant flags
>> >   migration: Remove duplicate vmstate macros
>> 
>> Nice work!
>> 
>> I think I was only looking at VBUFFER side and I thought it was fine
>> sticking with 32bit even signed or not, not a huge deal.  But cleaning up
>> VARRAY whole thing together looks definitely an improvement.  I definitely
>> like your version here.
>> 
>> I assume with your series I can drop both of my patches here, right?
>> 
>>   [PATCH v2 1/5] migration: Fix possible overflow in vmstate_handle_alloc()
>>   https://lore.kernel.org/r/[email protected]
>>   (I'll still respin with the rest)
>> 
>>   [PATCH] vhost/migration: Fix incorrect size used in inflight->addr in VMSD
>>   https://lore.kernel.org/r/[email protected]
>> 
>> The only missing piece would be an multiply overflow check in
>> vmstate_handle_alloc(), if you could add that check too while rewritting
>> that in patch 1 then I think it'll cover all.
>> 
>> Vladimir's ask in the separate email makes sense: I wonder if we can also
>> do one step further and merge VBUFFER into VARRAY.
>> 
>> The other trivial thing is, while looking, I found one trivial macro
>> VMSTATE_PARTIAL_VBUFFER not used; can drop it altogether.
>
> Now officially declare support for u64 on all these offsets, we also need
> to double check on our alignment with security issues.
>
> Similar reports will not be a bug anymore but results will be the same I
> assume: it's anything the attacker can feed a u64 directly (instead of an
> int32_t negative overflow), result is still failing a malloc() with
> enormously large numbers, legally this time.
>

Sprinkle some try_alloc maybe? Shouldn't be just a few from the
migration code itself.

> Do you still plan to work on finding per-user upper limit or whatever of
> that kind?  I'd say time spent working on series like this worths more than
> that, but I still want to check with you while looking at this solution.
>

I thought of some things for the future, but I don't have anything
concrete, only experiments:

1) For all vmstate macros that take an unbounded size, length, etc, add
a new variant:

 VMSTATE_FOO_BAR_V()
+VMSTATE_FOO_BAR_V_BOUNDED()

And allow the vmstate writer to provide a "Bounds" object (format to be
defined) and the vmstate core would just use that if it's there and
that's it. I still think that's a functionality the migration code
should have in its API.

2) Allowing a transfer limit in bytes to be set in QEMUFile, which can
be adjusted at will, maybe using some context-manager-like construct:

WITH_RX_CAP(f, 1024) {
    qemu_get_byte();
    qemu_get_byte();
    vmstate_load_vmsd()
    qemu_get_buf();
    ...
}

I'm still not sure how useful these would be.

> I suppose with this, one option is we can close all tickets reporting
> security issues while allocating with all u64 fields.
>
> Thanks,