On 15/05/23 8:28 pm, Daniel P. Berrangé wrote:
On Mon, May 15, 2023 at 08:08:57PM +0530, Het Gala wrote:
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.retnaka...@nutanix.com>
Signed-off-by: Het Gala <het.g...@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 ?
This will be validated later by getaddrinfo() so IMHO QEMU doesn't
need todo anythgin
Yes. I will keep it as it is for now. Thanks
With regards,
Daniel
Regards,
Het Gala

Reply via email to