RE: [v3] migration: fix the memory overwriting risk in add_to_iovec

[linfeng (M)]

* Dr. David Alan Gilbert(mailto:dgilbert@redhat.com) wrote:
> * Lin Feng (linfeng23@huawei.com) wrote:
> > From: Feng Lin <linfeng23@huawei.com>
> >
> > When testing migration, a Segmentation fault qemu core is generated.
> > 0  error_free (err=0x1)
> > 1  0x00007f8b862df647 in qemu_fclose (f=f@entry=0x55e06c247640)
> > 2  0x00007f8b8516d59a in migrate_fd_cleanup (s=s@entry=0x55e06c0e1ef0)
> > 3  0x00007f8b8516d66c in migrate_fd_cleanup_bh (opaque=0x55e06c0e1ef0)
> > 4  0x00007f8b8626a47f in aio_bh_poll (ctx=ctx@entry=0x55e06b5a16d0)
> > 5  0x00007f8b8626e71f in aio_dispatch (ctx=0x55e06b5a16d0)
> > 6  0x00007f8b8626a33d in aio_ctx_dispatch (source=<optimized out>, 
> > callback=<optimized out>,
> user_data=<optimized out>)
> > 7  0x00007f8b866bdba4 in g_main_context_dispatch ()
> > 8  0x00007f8b8626cde9 in glib_pollfds_poll ()
> > 9  0x00007f8b8626ce62 in os_host_main_loop_wait (timeout=<optimized out>)
> > 10 0x00007f8b8626cffd in main_loop_wait (nonblocking=nonblocking@entry=0)
> > 11 0x00007f8b862ef01f in main_loop ()
> > Using gdb print the struct QEMUFile f = {
> >   ...,
> >   iovcnt = 65, last_error = 21984,
> >   last_error_obj = 0x1, shutdown = true
> > }
> > Well iovcnt is overflow, because the max size of MAX_IOV_SIZE is 64.
> > struct QEMUFile {
> >     ...;
> >     struct iovec iov[MAX_IOV_SIZE];
> >     unsigned int iovcnt;
> >     int last_error;
> >     Error *last_error_obj;
> >     bool shutdown;
> > };
> > iovcnt and last_error is overwrited by add_to_iovec().
> > Right now, add_to_iovec() increase iovcnt before check the limit.
> > And it seems that add_to_iovec() assumes that iovcnt will set to zero
> > in qemu_fflush(). But qemu_fflush() will directly return when f->shutdown
> > is true.
> >
> > The situation may occur when libvirtd restart during migration, after
> > f->shutdown is set, before calling qemu_file_set_error() in
> > qemu_file_shutdown().
> >
> > So the safiest way is checking the iovcnt before increasing it.
> >
> > Signed-off-by: Feng Lin <linfeng23@huawei.com>
> > ---
> >  migration/qemu-file.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> > index d6e03dbc0e..f6486cf7bc 100644
> > --- a/migration/qemu-file.c
> > +++ b/migration/qemu-file.c
> > @@ -416,6 +416,9 @@ static int add_to_iovec(QEMUFile *f, const uint8_t 
> > *buf, size_t size,
> >      {
> >          f->iov[f->iovcnt - 1].iov_len += size;
> >      } else {
> > +        if (f->iovcnt >= MAX_IOV_SIZE) {
> > +            goto fflush;
> > +        }
> 
> Why call qemu_fflush in this case?
> If I understand what you're saying, then we only get to here if a
> previous qemu_fflush has failed, so this should fail as well?
Yes, that's what I mean.

> 
> How about, something like:
>     if (f->iovcnt >= MAX_IOV_SIZE) {
>         /* Should only happen if a previous fflush failed */
>         assert(f->shutdown || !qemu_file_is_writeable(f));
>         return 1;
>     }
> 
> ?
At first, I'm just thinking that overwriting requires qemu_fflush to reset 
iovcnt and do not consider
the possibility of packet loss caused by other exceptions. It makes more sense 
to make an assertion
here. Thank you for your suggestions.
> 
> Dave
> 
> >          if (may_free) {
> >              set_bit(f->iovcnt, f->may_free);
> >          }
> > @@ -423,12 +426,12 @@ static int add_to_iovec(QEMUFile *f, const uint8_t 
> > *buf, size_t size,
> >          f->iov[f->iovcnt++].iov_len = size;
> >      }
> >
> > -    if (f->iovcnt >= MAX_IOV_SIZE) {
> > -        qemu_fflush(f);
> > -        return 1;
> > +    if (f->iovcnt < MAX_IOV_SIZE) {
> > +        return 0;
> >      }
> > -
> > -    return 0;
> > +fflush:
> > +    qemu_fflush(f);
> > +    return 1;
> >  }
> >
> >  static void add_buf_to_iovec(QEMUFile *f, size_t len)
> > --
> > 2.23.0
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK