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

[Daniel P . Berrangé]

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);

> +
>      /*
>       * 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 

>      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
-- 
|: 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 :|