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

[Daniel P . Berrangé]

On Mon, Sep 14, 2020 at 05:20:14PM +0800, Zheng Chuan wrote:
> 
> 
> 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.

If MigrationState isn't safe to access from the multifd threads, then
don't addd it to the struct, as I think that will mislead people into
thinking it is ok to use. Only add the hostname.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|