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

["Daniel P. Berrangé" <berrange@redhat.com>]

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.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 ?

This will be validated later by getaddrinfo() so IMHO QEMU doesn't
need todo anythgin


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