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

[Yury Kotov]

Hi Vladimir!

13.09.2019, 16:43, "Vladimir Sementsov-Ogievskiy" <address@hidden>:
> 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.
>

Hmm.. It's very interesting, thanks! I need more time to understand
is there a problem or not, but after quick look I see one possibility
to achieve a leak: qmp_migrate after cleanup bh schedule and before bh call...

>>  +
>>    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

Regards,
Yury