[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] hostmem: Validate host-nodes before setting
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH v2] hostmem: Validate host-nodes before setting bitmap |
Date: |
Fri, 30 Nov 2018 12:27:42 -0200 |
User-agent: |
Mutt/1.9.2 (2017-12-15) |
On Fri, Nov 30, 2018 at 02:22:21PM +0100, Markus Armbruster wrote:
> Eduardo Habkost <address@hidden> 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 <address@hidden>
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> > 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 <address@hidden>
--
Eduardo