[PATCH 0/2] Fix crash if try to remove bitmap on target during migration

[PATCH 0/2] Fix crash if try to remove bitmap on target during migration

[Vladimir Sementsov-Ogievskiy]

Hi all!

Bitmaps on source are marked busy during migration.

Enabled bitmaps on target have successor, so they are busy.

But disabled migrated bitmaps are not protected on target. User can
simple remove them and it lead to use-after-free. These bitmaps should
be marked busy.

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 | 9 +++++++++
 2 files changed, 15 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 | 9 +++++++++
 1 file changed, 9 insertions(+)

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

Re: [PATCH 0/2] Fix crash if try to remove bitmap on target during migration

[Stefan Hajnoczi]

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

On Fri, Mar 19, 2021 at 11:41:22PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> Bitmaps on source are marked busy during migration.
> 
> Enabled bitmaps on target have successor, so they are busy.
> 
> But disabled migrated bitmaps are not protected on target. User can
> simple remove them and it lead to use-after-free. These bitmaps should
> be marked busy.
> 
> 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 | 9 +++++++++
>  2 files changed, 15 insertions(+)
> 
> -- 
> 2.29.2
> 

Thanks, applied to my block tree:
https://gitlab.com/stefanha/qemu/commits/block

Stefan

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

Re: [PATCH 0/2] Fix crash if try to remove bitmap on target during migration

[Vladimir Sementsov-Ogievskiy]

22.03.2021 14:28, Stefan Hajnoczi wrote:
> On Fri, Mar 19, 2021 at 11:41:22PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Bitmaps on source are marked busy during migration.
>>
>> Enabled bitmaps on target have successor, so they are busy.
>>
>> But disabled migrated bitmaps are not protected on target. User can
>> simple remove them and it lead to use-after-free. These bitmaps should
>> be marked busy.
>>
>> 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 | 9 +++++++++
>>   2 files changed, 15 insertions(+)
>>
>> -- 
>> 2.29.2
>>
> 
> Thanks, applied to my block tree:
> https://gitlab.com/stefanha/qemu/commits/block
> 
> Stefan
> 

Thanks!

O_o. Somehow, I've sent this thing twice, look at "[PATCH for-6.0 0/2] Fix use-after-free, if remove bitmap during migration". Sorry for the mess :\

patch 1 is the same, but patch 2 in new submission is updated to check that bitmaps can't be removed on source too. If it doesn't bother you can update the patch 2 in your branch too.

-- 
Best regards,
Vladimir

[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 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