Date: Fri, 30 Nov 2018 12:27:42 -0200
From: Eduardo Habkost <ehabkost@redhat.com>
Message-ID: <20181130142742.GG18284@habkost.net>
References: <20181130122844.29103-1-ehabkost@redhat.com>
	<87in0ek8du.fsf@dusky.pond.sub.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87in0ek8du.fsf@dusky.pond.sub.org>
Subject: Re: [Qemu-devel] [PATCH v2] hostmem: Validate host-nodes before
 setting bitmap
To: Markus Armbruster <armbru@redhat.com>
Cc: qemu-devel@nongnu.org, Igor Mammedov <imammedo@redhat.com>, Stefano Garzarella <sgarzare@redhat.com>, David Hildenbrand <david@redhat.com>

On Fri, Nov 30, 2018 at 02:22:21PM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > host_memory_backend_set_host_nodes() was not validating
> > host-nodes before writing to backend->host_nodes, making QEMU
> > write beyond the end of the bitmap.
> >
> > Fix the crash and add a simple regression test for the fix.
> >
> > While at it, fix memory leak of the list returned by
> > visit_type_uint16List().
> >
> > Reported-by: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Changes v1 -> v2:
> > * Remove extra `l = l->next` statement
> >   (reported by Stefano Garzarella)
> > * Fix (existing) leak of `host_nodes`
> >   (reported by Markus Armbruster)
> > ---
> >  backends/hostmem.c                   | 17 +++++++++----
> >  tests/acceptance/host-nodes-limit.py | 36 ++++++++++++++++++++++++++++
> >  2 files changed, 49 insertions(+), 4 deletions(-)
> >  create mode 100644 tests/acceptance/host-nodes-limit.py
> >
> > diff --git a/backends/hostmem.c b/backends/hostmem.c
> > index 1a89342039..af800284e0 100644
> > --- a/backends/hostmem.c
> > +++ b/backends/hostmem.c
> > @@ -103,14 +103,23 @@ host_memory_backend_set_host_nodes(Object *obj, Visitor *v, const char *name,
> >  {
> >  #ifdef CONFIG_NUMA
> >      HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> > -    uint16List *l = NULL;
> > +    uint16List *l, *host_nodes = NULL;
> >  
> > -    visit_type_uint16List(v, name, &l, errp);
> > +    visit_type_uint16List(v, name, &host_nodes, errp);
> >  
> > -    while (l) {
> > +    for (l = host_nodes; l; l = l->next) {
> > +        if (l->value >= MAX_NODES) {
> > +            error_setg(errp, "Invalid host-nodes value: %d", l->value);
> > +            goto out;
> > +        }
> > +    }
> > +
> > +    for (l = host_nodes; l; l = l->next) {
> >          bitmap_set(backend->host_nodes, l->value, 1);
> > -        l = l->next;
> >      }
> > +
> > +out:
> > +    qapi_free_uint16List(host_nodes);
> >  #else
> >      error_setg(errp, "NUMA node binding are not supported by this QEMU");
> >  #endif
> 
> Care to fix the leak in host_memory_backend_get_host_nodes(), too?

I don't understand yet if there's a leak at
host_memory_backend_get_host_nodes().  Won't
visit_type_uint16List() take ownership of the list on that case?

> 
> [...]
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

-- 
Eduardo