Peter Xu <[email protected]> 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 <[email protected]>
> ---
> 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;
