| Message ID | 1599965256-72150-4-git-send-email-zhengchuan@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | *** Add Multifd support for TLS migration *** | expand |
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
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.
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
On 2020/9/14 17:26, Daniel P. Berrangé wrote: > 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 > Sure, I'll fix that in V4. In addition, is that OK I pass hostname to MultiFDSendParams through MigrationState field as Patch001 do?
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; /* channel thread id */ QemuThread thread; /* communication channel */