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