Re: [PATCH 4/4] migration: Reset current_migration properly

[Fabiano Rosas <farosas@suse.de>]

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;