Re: [PATCH v4 4/8] migration: converts rdma backend to accept MigrateAddress struct

[Het Gala]

On 15/05/23 3:54 pm, Daniel P. Berrangé wrote:
> On Fri, May 12, 2023 at 02:32:36PM +0000, Het Gala wrote:
> > RDMA based transport backend for 'migrate'/'migrate-incoming' QAPIs
> > accept new wire protocol of MigrateAddress struct.
> >
> > It is achived by parsing 'uri' string and storing migration parameters
> > required for RDMA connection into well defined InetSocketAddress struct.
> >
> > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> > Signed-off-by: Het Gala <het.gala@nutanix.com>
> > ---
> >   migration/migration.c |  8 ++++----
> >   migration/rdma.c      | 38 ++++++++++++++++----------------------
> >   migration/rdma.h      |  6 ++++--
> >   3 files changed, 24 insertions(+), 28 deletions(-)
> >
> > @@ -3360,10 +3346,12 @@ static int qemu_rdma_accept(RDMAContext *rdma)
> >                                               .private_data_len = sizeof(cap),
> >                                            };
> >       RDMAContext *rdma_return_path = NULL;
> > +    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
> >       struct rdma_cm_event *cm_event;
> >       struct ibv_context *verbs;
> >       int ret = -EINVAL;
> >       int idx;
> > +    char arr[8];
> >   
> >       ret = rdma_get_cm_event(rdma->channel, &cm_event);
> >       if (ret) {
> > @@ -3375,13 +3363,17 @@ static int qemu_rdma_accept(RDMAContext *rdma)
> >           goto err_rdma_dest_wait;
> >       }
> >   
> > +    isock->host = rdma->host;
> > +    sprintf(arr,"%d", rdma->port);
> > +    isock->port = arr;
> While Inet ports are 16-bit, and so 65535 fits in a char[8], nothing
> at the QAPI parser level is enforcing this.
>
> IOW, someone can pass QEMU a QAPI config with port = 235252353253253253232
> and casue this sprintf to smash the stack.
>
> Also this is assigning a stack variable to isock->port which
> expects a heap variable. qapi_free_InetSocketAddress() will
> call free(isock->port) which will again crash.
>
> Just do
>
>    g_autoptr(InetSocketAddress) isock = g_new0(InetSocketAddress, 1);
>
>    isock->port = g_strdup_printf("%d", rdma->port);
Thanks Daniel. Will change this in next version of patchset. Is a 
protection for isock->host and isock->port needed here ?
> > +
> >       /*
> >        * initialize the RDMAContext for return path for postcopy after first
> >        * connection request reached.
> >        */
> >       if ((migrate_postcopy() || migrate_return_path())
> >           && !rdma->is_return_path) {
> > -        rdma_return_path = qemu_rdma_data_init(rdma->host_port, NULL);
> > +        rdma_return_path = qemu_rdma_data_init(isock, NULL);
> >           if (rdma_return_path == NULL) {
> >               rdma_ack_cm_event(cm_event);
> >               goto err_rdma_dest_wait;
> > @@ -3506,6 +3498,8 @@ static int qemu_rdma_accept(RDMAContext *rdma)
> >   err_rdma_dest_wait:
> >       rdma->error_state = ret;
> >       qemu_rdma_cleanup(rdma);
> > +    qapi_free_InetSocketAddress(isock);
> > +    g_free(arr);
> Free'ing a stack variable
Ack, will delete both statements from here.
> >       g_free(rdma_return_path);
> >       return ret;
> >   }
> > @@ -4114,7 +4108,8 @@ static void rdma_accept_incoming_migration(void *opaque)
> >       }
> >   }
> >   
> > -void rdma_start_incoming_migration(const char *host_port, Error **errp)
> > +void rdma_start_incoming_migration(InetSocketAddress *host_port,
> > +                                   Error **errp)
> >   {
> >       int ret;
> >       RDMAContext *rdma;
> > @@ -4160,13 +4155,12 @@ err:
> >       error_propagate(errp, local_err);
> >       if (rdma) {
> >           g_free(rdma->host);
> > -        g_free(rdma->host_port);
> >       }
> >       g_free(rdma);
> >   }
> >   
> >   void rdma_start_outgoing_migration(void *opaque,
> > -                            const char *host_port, Error **errp)
> > +                            InetSocketAddress *host_port, Error **errp)
> >   {
> >       MigrationState *s = opaque;
> >       RDMAContext *rdma_return_path = NULL;
> > diff --git a/migration/rdma.h b/migration/rdma.h
> > index de2ba09dc5..ee89296555 100644
> > --- a/migration/rdma.h
> > +++ b/migration/rdma.h
> > @@ -14,12 +14,14 @@
> >    *
> >    */
> >   
> > +#include "qemu/sockets.h"
> > +
> >   #ifndef QEMU_MIGRATION_RDMA_H
> >   #define QEMU_MIGRATION_RDMA_H
> >   
> > -void rdma_start_outgoing_migration(void *opaque, const char *host_port,
> > +void rdma_start_outgoing_migration(void *opaque, InetSocketAddress *host_port,
> >                                      Error **errp);
> >   
> > -void rdma_start_incoming_migration(const char *host_port, Error **errp);
> > +void rdma_start_incoming_migration(InetSocketAddress *host_port, Error **errp);
> >   
> >   #endif
> > --
> > 2.22.3
> With regards,
> Daniel
Regards,
Het Gala