[PATCH for-6.0 0/2] Fix use-after-free, if remove bitmap during migration

[PATCH for-6.0 0/2] Fix use-after-free, if remove bitmap during migration

[Vladimir Sementsov-Ogievskiy]

Hi all! Accidentally we found on use-after-free. Normally user should
not remove bitmaps during migration.. But some wrong user actions may
simply lead to Qemu crash and that's not good.

Vladimir Sementsov-Ogievskiy (2):
  migration/block-dirty-bitmap: make incoming disabled bitmaps busy
  migrate-bitmaps-postcopy-test: check that we can't remove in-flight
    bitmaps

 migration/block-dirty-bitmap.c                         |  6 ++++++
 tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test | 10 ++++++++++
 2 files changed, 16 insertions(+)

-- 
2.29.2

[PATCH 1/2] migration/block-dirty-bitmap: make incoming disabled bitmaps busy

[Vladimir Sementsov-Ogievskiy]

Incoming enabled bitmaps are busy, because we do
bdrv_dirty_bitmap_create_successor() for them. But disabled bitmaps
being migrated are not marked busy, and user can remove them during the
incoming migration. Then we may crash in cancel_incoming_locked() when
try to remove the bitmap that was already removed by user, like this:

 #0  qemu_mutex_lock_impl (mutex=0x5593d88c50d1, file=0x559680554b20
   "../block/dirty-bitmap.c", line=64) at ../util/qemu-thread-posix.c:77
 #1  bdrv_dirty_bitmaps_lock (bs=0x5593d88c0ee9)
   at ../block/dirty-bitmap.c:64
 #2  bdrv_release_dirty_bitmap (bitmap=0x5596810e9570)
   at ../block/dirty-bitmap.c:362
 #3  cancel_incoming_locked (s=0x559680be8208 <dbm_state+40>)
   at ../migration/block-dirty-bitmap.c:918
 #4  dirty_bitmap_load (f=0x559681d02b10, opaque=0x559680be81e0
   <dbm_state>, version_id=1) at ../migration/block-dirty-bitmap.c:1194
 #5  vmstate_load (f=0x559681d02b10, se=0x559680fb5810)
   at ../migration/savevm.c:908
 #6  qemu_loadvm_section_part_end (f=0x559681d02b10,
   mis=0x559680fb4a30) at ../migration/savevm.c:2473
 #7  qemu_loadvm_state_main (f=0x559681d02b10, mis=0x559680fb4a30)
   at ../migration/savevm.c:2626
 #8  postcopy_ram_listen_thread (opaque=0x0)
   at ../migration/savevm.c:1871
 #9  qemu_thread_start (args=0x5596817ccd10)
   at ../util/qemu-thread-posix.c:521
 #10 start_thread () at /lib64/libpthread.so.0
 #11 clone () at /lib64/libc.so.6

Note bs pointer taken from bitmap: it's definitely bad aligned. That's
because we are in use after free, bitmap is already freed.

So, let's make disabled bitmaps (being migrated) busy during incoming
migration.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 migration/block-dirty-bitmap.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 975093610a..35f5ef688d 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -839,6 +839,8 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
             error_report_err(local_err);
             return -EINVAL;
         }
+    } else {
+        bdrv_dirty_bitmap_set_busy(s->bitmap, true);
     }
 
     b = g_new(LoadBitmapState, 1);
@@ -914,6 +916,8 @@ static void cancel_incoming_locked(DBMLoadState *s)
         assert(!s->before_vm_start_handled || !b->migrated);
         if (bdrv_dirty_bitmap_has_successor(b->bitmap)) {
             bdrv_reclaim_dirty_bitmap(b->bitmap, &error_abort);
+        } else {
+            bdrv_dirty_bitmap_set_busy(b->bitmap, false);
         }
         bdrv_release_dirty_bitmap(b->bitmap);
     }
@@ -951,6 +955,8 @@ static void dirty_bitmap_load_complete(QEMUFile *f, DBMLoadState *s)
 
     if (bdrv_dirty_bitmap_has_successor(s->bitmap)) {
         bdrv_reclaim_dirty_bitmap(s->bitmap, &error_abort);
+    } else {
+        bdrv_dirty_bitmap_set_busy(s->bitmap, false);
     }
 
     for (item = s->bitmaps; item; item = g_slist_next(item)) {
-- 
2.29.2

[PATCH 2/2] migrate-bitmaps-postcopy-test: check that we can't remove in-flight bitmaps

[Vladimir Sementsov-Ogievskiy]

Check that we can't remove bitmaps being migrated on destination vm.
The new check proves that previous commit helps.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
index d046ebeb94..584062b412 100755
--- a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
+++ b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
@@ -224,6 +224,16 @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):
         self.start_postcopy()
 
         self.vm_b_events += self.vm_b.get_qmp_events()
+
+        # While being here, let's check that we can't remove in-flight bitmaps.
+        for vm in (self.vm_a, self.vm_b):
+            for i in range(0, nb_bitmaps):
+                result = vm.qmp('block-dirty-bitmap-remove', node='drive0',
+                                name=f'bitmap{i}')
+                self.assert_qmp(result, 'error/desc',
+                                f"Bitmap 'bitmap{i}' is currently in use by "
+                                "another operation and cannot be used")
+
         self.vm_b.shutdown()
         # recreate vm_b, so there is no incoming option, which prevents
         # loading bitmaps from disk
-- 
2.29.2

Re: [PATCH for-6.0 0/2] Fix use-after-free, if remove bitmap during migration

[Vladimir Sementsov-Ogievskiy]

22.03.2021 12:49, Vladimir Sementsov-Ogievskiy wrote:
> Hi all! Accidentally we found on use-after-free. Normally user should
> not remove bitmaps during migration.. But some wrong user actions may
> simply lead to Qemu crash and that's not good.
> 
> Vladimir Sementsov-Ogievskiy (2):
>    migration/block-dirty-bitmap: make incoming disabled bitmaps busy
>    migrate-bitmaps-postcopy-test: check that we can't remove in-flight
>      bitmaps
> 
>   migration/block-dirty-bitmap.c                         |  6 ++++++
>   tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test | 10 ++++++++++
>   2 files changed, 16 insertions(+)
> 

Oops sorry. Actually, it's a v2 for "[PATCH 0/2] Fix crash if try to remove bitmap on target during migration" with a bit improved test, patch 1` unchanged.

Supersedes: <20210319204124.364312-1-vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH for-6.0 0/2] Fix use-after-free, if remove bitmap during migration

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 825 bytes --]

On Mon, Mar 22, 2021 at 12:49:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all! Accidentally we found on use-after-free. Normally user should
> not remove bitmaps during migration.. But some wrong user actions may
> simply lead to Qemu crash and that's not good.
> 
> Vladimir Sementsov-Ogievskiy (2):
>   migration/block-dirty-bitmap: make incoming disabled bitmaps busy
>   migrate-bitmaps-postcopy-test: check that we can't remove in-flight
>     bitmaps
> 
>  migration/block-dirty-bitmap.c                         |  6 ++++++
>  tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test | 10 ++++++++++
>  2 files changed, 16 insertions(+)
> 
> -- 
> 2.29.2
> 

Thanks, applied to my cpuidle-haltpoll-virtqueue tree:
https://gitlab.com/stefanha/qemu/commits/cpuidle-haltpoll-virtqueue

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]