Re: [Qemu-devel] [PULL 10/28] Sanity check RDMA remote data

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 07/07/2015 15:08, Juan Quintela wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Perform some basic (but probably not complete) sanity checking on
> > requests from the RDMA source.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Reviewed-by: Michael R. Hines <mrhines@us.ibm.com>
> > Signed-off-by: Juan Quintela <quintela@redhat.com>
> > ---
> >  migration/rdma.c | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/migration/rdma.c b/migration/rdma.c
> > index 73844a3..73a79be 100644
> > --- a/migration/rdma.c
> > +++ b/migration/rdma.c
> > @@ -2992,6 +2992,13 @@ static int qemu_rdma_registration_handle(QEMUFile *f, void *opaque)
> >              trace_qemu_rdma_registration_handle_compress(comp->length,
> >                                                           comp->block_idx,
> >                                                           comp->offset);
> > +            if (comp->block_idx >= rdma->local_ram_blocks.nb_blocks) {
> > +                error_report("rdma: 'compress' bad block index %u (vs %d)",
> > +                             (unsigned int)comp->block_idx,
> > +                             rdma->local_ram_blocks.nb_blocks);
> > +                ret = -EIO;
> > +                break;
> 
> Did you want "goto out" here, and especially inside the for loop below:

Yes, it was nice of Coverity to spot that.

> > +            }
> >              block = &(rdma->local_ram_blocks.block[comp->block_idx]);
> > 
> >              host_addr = block->local_host_addr +
> > @@ -3080,8 +3087,23 @@ static int qemu_rdma_registration_handle(QEMUFile *f, void *opaque)
> >                  trace_qemu_rdma_registration_handle_register_loop(count,
> >                           reg->current_index, reg->key.current_addr, reg->chunks);
> > 
> > +                if (reg->current_index >= rdma->local_ram_blocks.nb_blocks) {
> > +                    error_report("rdma: 'register' bad block index %u (vs %d)",
> > +                                 (unsigned int)reg->current_index,
> > +                                 rdma->local_ram_blocks.nb_blocks);
> > +                    ret = -ENOENT;
> > +                    break;
> 
> Here
> 
> > +                }
> >                  block = &(rdma->local_ram_blocks.block[reg->current_index]);
> >                  if (block->is_ram_block) {
> > +                    if (block->offset > reg->key.current_addr) {
> > +                        error_report("rdma: bad register address for block %s"
> > +                            " offset: %" PRIx64 " current_addr: %" PRIx64,
> > +                            block->block_name, block->offset,
> > +                            reg->key.current_addr);
> > +                        ret = -ERANGE;
> > +                        break;
> 
> here
> 
> > +                    }
> >                      host_addr = (block->local_host_addr +
> >                                  (reg->key.current_addr - block->offset));
> >                      chunk = ram_chunk_index(block->local_host_addr,
> > @@ -3090,6 +3112,14 @@ static int qemu_rdma_registration_handle(QEMUFile *f, void *opaque)
> >                      chunk = reg->key.chunk;
> >                      host_addr = block->local_host_addr +
> >                          (reg->key.chunk * (1UL << RDMA_REG_CHUNK_SHIFT));
> > +                    /* Check for particularly bad chunk value */
> > +                    if (host_addr < (void *)block->local_host_addr) {
> > +                        error_report("rdma: bad chunk for block %s"
> > +                            " chunk: %" PRIx64,
> > +                            block->block_name, reg->key.chunk);
> > +                        ret = -ERANGE;
> > +                        break;
> 
> and here the "break" takes you directly to
> 
>    ret = qemu_rdma_post_send_control(rdma,
>                             (uint8_t *) results, &reg_resp);
> 
> where ret is overwritten (spotted by Coverity).

Yes, it needs the goto's back.

Dave

> 
> Paolo
> 
> 
> Paolo
> 
> > +                    }
> >                  }
> >                  chunk_start = ram_chunk_start(block, chunk);
> >                  chunk_end = ram_chunk_end(block, chunk + reg->chunks);
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK