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 <zhengch...@huawei.com>
> >> Signed-off-by: Yan Jin <jinya...@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 :|
