Re: [PATCH v3 15/21] migration/block-dirty-bitmap: relax error handling in incoming part

[Dr. David Alan Gilbert]

* Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
> 24.07.2020 20:35, Eric Blake wrote:
> > On 7/24/20 3:43 AM, Vladimir Sementsov-Ogievskiy wrote:
> > > Bitmaps data is not critical, and we should not fail the migration (or
> > > use postcopy recovering) because of dirty-bitmaps migration failure.
> > > Instead we should just lose unfinished bitmaps.
> > > 
> > > Still we have to report io stream violation errors, as they affect the
> > > whole migration stream.
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > ---
> > > Â  migration/block-dirty-bitmap.c | 152 +++++++++++++++++++++++++--------
> > > Â  1 file changed, 117 insertions(+), 35 deletions(-)
> > > 
> > 
> > > @@ -650,15 +695,32 @@ static int dirty_bitmap_load_bits(QEMUFile *f, 
> > > DBMLoadState *s)
> > > Â Â Â Â Â  if (s->flags & DIRTY_BITMAP_MIG_FLAG_ZEROES) {
> > > Â Â Â Â Â Â Â Â Â  trace_dirty_bitmap_load_bits_zeroes();
> > > -Â Â Â Â Â Â Â  bdrv_dirty_bitmap_deserialize_zeroes(s->bitmap, 
> > > first_byte, nr_bytes,
> > > -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â 
> > > Â Â Â Â Â Â Â Â  false);
> > > +Â Â Â Â Â Â Â  if (!s->cancelled) {
> > > +Â Â Â Â Â Â Â Â Â Â Â  bdrv_dirty_bitmap_deserialize_zeroes(s->bitmap, 
> > > first_byte,
> > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â 
> > > Â Â Â Â Â Â Â Â Â Â Â Â  nr_bytes, false);
> > > +Â Â Â Â Â Â Â  }
> > > Â Â Â Â Â  } else {
> > > Â Â Â Â Â Â Â Â Â  size_t ret;
> > > Â Â Â Â Â Â Â Â Â  uint8_t *buf;
> > > Â Â Â Â Â Â Â Â Â  uint64_t buf_size = qemu_get_be64(f);
> > 
> > Pre-existing, but if I understand, we are reading a value from the 
> > migration stream...
> 
> Hmm, actually, this becomes worse after patch, as before patch we had the 
> check, that the size at least corresponds to the bitmap.. But we want to 
> relax things in cancelled mode (and we may not have any bitmap). Most correct 
> thing is to use read in a loop to just skip the data from stream if we are in 
> cancelled mode (something like nbd_drop()).
> 
> I can fix this with a follow-up patch.

If the size is bogus, it's probably not worth trying to skip anything
because it could be just a broken/misaligned stream.

Dave

> > 
> > > -Â Â Â Â Â Â Â  uint64_t needed_size =
> > > -Â Â Â Â Â Â Â Â Â Â Â  bdrv_dirty_bitmap_serialization_size(s->bitmap,
> > > -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â 
> > > Â Â Â Â Â Â Â Â Â Â Â Â  first_byte, nr_bytes);
> > > +Â Â Â Â Â Â Â  uint64_t needed_size;
> > > +
> > > +Â Â Â Â Â Â Â  buf = g_malloc(buf_size);
> > > +Â Â Â Â Â Â Â  ret = qemu_get_buffer(f, buf, buf_size);
> > 
> > ...and using it to malloc memory.  Is that a potential risk of a malicious 
> > stream causing us to allocate too much memory in relation to the guest's 
> > normal size?  If so, fixing that should be done separately.
> > 
> > I'm not a migration expert, but the patch looks reasonable to me.
> > 
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > 
> 
> 
> -- 
> Best regards,
> Vladimir
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK