Re: [PATCH v3 3/6] migration/tls: add MigrationState and tls_hostname into MultiFDSendParams

[Zheng Chuan]

On 2020/9/14 17:02, Daniel P. Berrangé wrote:
> On Sun, Sep 13, 2020 at 10:47:33AM +0800, Chuan Zheng wrote:
>> MigrationState is need for tls session build and tls hostname is need
>> for tls handshake, add both MigrationState and tls_hostname
>> into MultiFDSendParams.
>>
>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>> Signed-off-by: Yan Jin <jinyan12@huawei.com>
>> ---
>>  migration/multifd.c | 5 +++++
>>  migration/multifd.h | 4 ++++
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index d044120..3e41d9e 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -543,11 +543,14 @@ void multifd_save_cleanup(void)
>>  
>>          socket_send_channel_destroy(p->c);
>>          p->c = NULL;
>> +        p->s = NULL;
>>          qemu_mutex_destroy(&p->mutex);
>>          qemu_sem_destroy(&p->sem);
>>          qemu_sem_destroy(&p->sem_sync);
>>          g_free(p->name);
>>          p->name = NULL;
>> +        g_free(p->tls_hostname);
>> +        p->tls_hostname = NULL;
>>          multifd_pages_clear(p->pages);
>>          p->pages = NULL;
>>          p->packet_len = 0;
>> @@ -779,6 +782,8 @@ int multifd_save_setup(Error **errp)
>>          p->packet->magic = cpu_to_be32(MULTIFD_MAGIC);
>>          p->packet->version = cpu_to_be32(MULTIFD_VERSION);
>>          p->name = g_strdup_printf("multifdsend_%d", i);
>> +        p->s = migrate_get_current();
>> +        p->tls_hostname = g_strdup(p->s->hostname);
>>          socket_send_channel_create(multifd_new_send_channel_async, p);
>>      }
>>  
>> diff --git a/migration/multifd.h b/migration/multifd.h
>> index 448a03d..2b400e7 100644
>> --- a/migration/multifd.h
>> +++ b/migration/multifd.h
>> @@ -66,11 +66,15 @@ typedef struct {
>>  } MultiFDPages_t;
>>  
>>  typedef struct {
>> +    /* Migration State */
>> +    MigrationState *s;
>>      /* this fields are not changed once the thread is created */
>>      /* channel number */
>>      uint8_t id;
>>      /* channel thread name */
>>      char *name;
>> +    /* tls hostname */
>> +    char *tls_hostname;
> 
> Why do we need this, when it is already accessible from the
> MigrationState field you're adding
> 
> 
> Regards,
> Daniel
> 
Hi，Daniel. Thank you for your review.

This is because i have free hostname in MigrationState field after 
migrate_fd_connect(s, error).
Since multifd thread creation is async by socket_send_channel_create(), we must 
record it in MultiFDSendParams
in case of concurrency issues.

migration_channel_connect
       migrate_fd_connect
           multifd_save_setup
              socket_send_channel_create(multifd_new_send_channel_async, p); / 
async, do not wait for multifd creation
                g_free(s->hostname);
                                                          
multifd_new_send_channel_async
                                                                  
multifd_channel_connect
                                                                         
multifd_tls_channel_connect
                                                                                
migration_tls_client_create  /* UAF happen */

As you mentioned in Patch001, i am not sure if it will cause the same 
concurrency issues if i put hostname in MigrationState field
freed in migrate_fd_cancel.