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

[Het Gala <het.gala@nutanix.com>]

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