Re: [Qemu-devel] [PATCH] migration: Fix use-after-free during process exit

[Vladimir Sementsov-Ogievskiy]

Hi!

08.04.2019 14:33, Yury Kotov wrote:
> It fixes heap-use-after-free which was found by clang's ASAN.
> 
> Control flow of this use-after-free:
> main_thread:
>      * Got SIGTERM and completes main loop
>      * Calls migration_shutdown
>        - migrate_fd_cancel (so, migration_thread begins to complete)
>        - object_unref(OBJECT(current_migration));
> 
> migration_thread:
>      * migration_iteration_finish -> schedule cleanup bh
>      * object_unref(OBJECT(s)); (Now, current_migration is freed)
>      * exits
> 
> main_thread:
>      * Calls vm_shutdown -> drain bdrvs -> main loop
>        -> cleanup_bh -> use after free
> 


[..]

>   
> +static void migrate_fd_cleanup_schedule(MigrationState *s)
> +{
> +    /* Ref the state for bh, because it may be called when
> +     * there're already no other refs */
> +    object_ref(OBJECT(s));
> +    qemu_bh_schedule(s->cleanup_bh);
> +}

so you ref on scheduling

> +
> +static void migrate_fd_cleanup_bh(void *opaque)
> +{
> +    MigrationState *s = opaque;
> +    migrate_fd_cleanup(s);
> +    object_unref(OBJECT(s));
> +}

and unref after execution of bh.

but migration code has also call to qemu_bh_delete(s->cleanup_bh) from
migrate_fd_cleanup(), in migrate_fd_cleanup() is called not only from
migrate_fd_cleanup_bh..

I mean, that if we call qemu_bh_delete after scheduling, we will not
unref our new reference.

Still, seems it's impossible, as all other calls to migrate_fd_cleanup
are done before migration thread creation.

Don't know, should something done around it or not, but decided to mention it.


> +
>   void migrate_set_error(MigrationState *s, const Error *error)
>   {
>       qemu_mutex_lock(&s->error_mutex);
> @@ -3144,7 +3157,7 @@ static void migration_iteration_finish(MigrationState 
> *s)
>           error_report("%s: Unknown ending state %d", __func__, s->state);
>           break;
>       }
> -    qemu_bh_schedule(s->cleanup_bh);
> +    migrate_fd_cleanup_schedule(s);
>       qemu_mutex_unlock_iothread();
>   }
>   
> @@ -3279,7 +3292,7 @@ void migrate_fd_connect(MigrationState *s, Error 
> *error_in)
>       bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
>   
>       s->expected_downtime = s->parameters.downtime_limit;
> -    s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup, s);
> +    s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup_bh, s);
>       if (error_in) {
>           migrate_fd_error(s, error_in);
>           migrate_fd_cleanup(s);
> 




-- 
Best regards,
Vladimir