On Wed, Aug 19, 2026 at 5:21 PM Peter Xu <[email protected]> wrote:
>
> On Wed, Aug 19, 2026 at 03:00:51PM +0200, Jinpu Wang wrote:
> > On Mon, Aug 17, 2026 at 10:24 PM Peter Xu <[email protected]> wrote:
> > >
> > > The value received on wire for head.chunks when registering new RDMA
> > > regions is not correctly checked. Logically the value can still make
> > > ram_chunk_start() (of ram_chunk_end()) to overflow, having a result
> > > pointer
> > > very small, smaller than RDMALocalBlock.local_host_addr.
> > >
> > > Add the sanity check.
> > >
> > > Reported-by: Tristan (@TristanInSec)
> > > Closes: https://gitlab.com/qemu-project/qemu/-/work_items/4011
> > > Signed-off-by: Peter Xu <[email protected]>
> > > ---
> > > migration/rdma.c | 7 +++++++
> > > 1 file changed, 7 insertions(+)
> > >
> > > diff --git a/migration/rdma.c b/migration/rdma.c
> > > index 5ce8b06818..bbbc40ea3b 100644
> > > --- a/migration/rdma.c
> > > +++ b/migration/rdma.c
> > > @@ -3401,6 +3401,13 @@ int rdma_registration_handle(QEMUFile *f)
> > > chunk = ram_chunk_index(block->local_host_addr,
> > > (uint8_t *) host_addr);
> > > chunk_start = ram_chunk_start(block, chunk);
> > > + if (chunk + reg->chunks > block->nb_chunks) {
> > > + error_report("%s: head.chunks contains illegal value"
> > > + " (chunk=%"PRIu64", chunks=%"PRIu64", "
> > > + "nb_chunks=%d)", __func__, chunk,
> > > + reg->chunks, block->nb_chunks);
> > > + goto err;
> > > + }
> > safer to do this instead, as reg->chunks is from wire:
> > uint64_t chunk_sum;
> > if (uadd64_overflow(chunk, reg->chunks, &chunk_sum) ||
> > chunk_sum > block->nb_chunks) {
> > error_report(...);
> > goto err;
> > }
>
> I thought I covered that, I even mentioned the overflow in the commit log,
> but I didn't really do anything..
>
> When looking at this again, I found reg->chunks is defined in a weird way,
> instead of "number of chunks", it's off-by-one...
>
> qemu_rdma_write_one():
> chunks = length / chunk_size;
> if (chunks && ((length % chunk_size) == 0)) {
> chunks--;
> }
>
> So I think I should check it with "chunk_sum >= block->nb_chunks" to be
> accurate. Diff attached to be squashed when repost, please help double
> check:
Yes, looks indeed.
>
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 6384306ead..e976739fad 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -3393,7 +3393,7 @@ int rdma_registration_handle(QEMUFile *f)
> }
>
> for (int count = 0; count < head.repeat; count++) {
> - uint64_t chunk;
> + uint64_t chunk, chunk_sum;
> uint8_t *chunk_start, *chunk_end;
>
> reg = ®isters[count];
> @@ -3424,7 +3424,8 @@ int rdma_registration_handle(QEMUFile *f)
> chunk = ram_chunk_index(block->local_host_addr,
> (uint8_t *) host_addr);
> chunk_start = ram_chunk_start(block, chunk);
> - if (chunk + reg->chunks > block->nb_chunks) {
> + if (uadd64_overflow(chunk, reg->chunks, &chunk_sum) ||
> + chunk_sum >= block->nb_chunks) {
> error_report("%s: head.chunks contains illegal value"
> " (chunk=%"PRIu64", chunks=%"PRIu64", "
> "nb_chunks=%d)", __func__, chunk,
>
lgtm.
Reviewed-by: Jack Wang <[email protected]>
> Thanks for the careful review,
Thank you for taking care.
>
> --
> Peter Xu
>
