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 the only way to do is via
finalize() as we know there's only one instance of it, meanwhile QEMU won't
know who holds the refcount, so it can't reset the variable manually but
only in finalize().

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

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 1b5285af95..74812ca785 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -233,9 +233,11 @@ 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));
+    /* This creates the singleton migration object */
+    object_new(TYPE_MIGRATION);
+
+    /* This should be set now when initialize the singleton object */
+    assert(current_migration);
 
     /*
      * Init the migrate incoming object as well no matter whether
@@ -3886,12 +3888,27 @@ 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().
+     */
+    assert(!current_migration);
+    current_migration = ms;
+
     ms->state = MIGRATION_STATUS_NONE;
     ms->mbps = -1;
     ms->pages_per_second = -1;
-- 
2.45.0

Peter Xu <peterx@redhat.com> writes:

> 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 the only way to do is via
> finalize() as we know there's only one instance of it, meanwhile QEMU won't
> know who holds the refcount, so it can't reset the variable manually but
> only in finalize().
>
> To make it more readable, also initialize the variable in the
> instance_init() so it's very well paired at least.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 1b5285af95..74812ca785 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -233,9 +233,11 @@ 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));
> +    /* This creates the singleton migration object */
> +    object_new(TYPE_MIGRATION);
> +
> +    /* This should be set now when initialize the singleton object */
> +    assert(current_migration);
>  
>      /*
>       * Init the migrate incoming object as well no matter whether
> @@ -3886,12 +3888,27 @@ 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

instance

> +     * here it means migration object is gone.  Clear the global reference
> +     * to reflect that.

Not really gone at this point. The free only happens when this function
returns.

> +     */
> +    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().
> +     */
> +    assert(!current_migration);
> +    current_migration = ms;
> +
>      ms->state = MIGRATION_STATUS_NONE;
>      ms->mbps = -1;
>      ms->pages_per_second = -1;

On Thu, Oct 24, 2024 at 04:34:44PM -0300, Fabiano Rosas wrote:
> > +     * here it means migration object is gone.  Clear the global reference
> > +     * to reflect that.
> 
> Not really gone at this point. The free only happens when this function
> returns.

How about "is going away"?

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Thu, Oct 24, 2024 at 04:34:44PM -0300, Fabiano Rosas wrote:
>> > +     * here it means migration object is gone.  Clear the global reference
>> > +     * to reflect that.
>> 
>> Not really gone at this point. The free only happens when this function
>> returns.
>
> How about "is going away"?

Yep