Re: [PATCH 08/10] migration/rdma: Sanity check RDMA_CONTROL_REGISTER_REQUEST chunks

[Peter Xu <peterx@redhat.com>]

On Wed, Aug 19, 2026 at 03:00:51PM +0200, Jinpu Wang wrote:
> On Mon, Aug 17, 2026 at 10:24 PM Peter Xu <peterx@redhat.com> 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 <peterx@redhat.com>
> > ---
> >  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:

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 = &registers[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,

Thanks for the careful review,

-- 
Peter Xu