[v2,2/4] migration: Reset current_migration properly

Commit Message

Peter Xu Oct. 23, 2024, 6:02 p.m. UTC

current_migration is never reset, even if the migration object is freed
already.  It means anyone references that can trigger UAF and it'll be hard
to debug.

Properly clear the pointer now, so far by doing it in the finalize() (as we
know there's only one instance of it).

Add a TODO entry for it showing that we can do better in the future.

To make it clear, also initialize the variable in the instance_init() so
it's very well paired at least.

Signed-off-by: Peter Xu <[e-mail]>
---
 migration/migration.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

Patch

diff --git a/migration/migration.c b/migration/migration.c
index bcb735869b..a82297db0f 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -232,9 +232,9 @@  static int migration_stop_vm(MigrationState *s, RunState state)
 
 void migration_object_init(void)
 {
-    /* This can only be called once. */
-    assert(!current_migration);
-    current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
+    MIGRATION_OBJ(object_new(TYPE_MIGRATION));
+    /* This should be set when initialize the object */
+    assert(current_migration);
 
     /*
      * Init the migrate incoming object as well no matter whether
@@ -3877,12 +3877,31 @@  static void migration_instance_finalize(Object *obj)
     qemu_sem_destroy(&ms->rp_state.rp_pong_acks);
     qemu_sem_destroy(&ms->postcopy_qemufile_src_sem);
     error_free(ms->error);
+
+    /*
+     * We know we only have one intance of migration, and when reaching
+     * here it means migration object is gone.  Clear the global reference
+     * to reflect that.
+     */
+    current_migration = NULL;
 }
 
 static void migration_instance_init(Object *obj)
 {
     MigrationState *ms = MIGRATION_OBJ(obj);
 
+    /*
+     * There can only be one migration object globally. Keep a record of
+     * the pointer in current_migration, which will be reset after the
+     * object finalize().
+     *
+     * TODO: after migration/ code can always take a MigrationObject*
+     * pointer all over the place, logically we can drop current_migration
+     * variable.
+     */
+    assert(!current_migration);
+    current_migration = ms;
+
     ms->state = MIGRATION_STATUS_NONE;
     ms->mbps = -1;
     ms->pages_per_second = -1;