hostmem: Validate host-nodes before setting bitmap
PATCH 1/1 hostmem: Validate host-nodes before setting bitmap
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.
--- backends/hostmem.c | 13 +++++++--- tests/acceptance/host-nodes-limit.py | 36 ++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) create mode 100644 tests/acceptance/host-nodes-limit.py
backends/hostmem.c +10 -3
@@ -103,11 +103,18 @@ 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); + +for (l = host_nodes; l; l = l->next) { +if (l->value >=MAX_NODES ) { +error_setg (errp,"Invalid host-nodes value: %d" , l->value); +return ; + } + } -while (l) { +for (l = host_nodes; l; l = l->next) {bitmap_set (backend->host_nodes, l->value,1 ); l = l->next; }
tests/acceptance/host-nodes-limit.py new file +36 -0
@@ -0,0 +1,36 @@ +# Regression test for host-nodes limit validation +# +# Copyright (c) 2018 Red Hat, Inc. +# +# Author: +# Eduardo Habkost <[email protected]> +# +# This work is licensed under the terms of the GNU GPL, version 2 or +# later. See the COPYING file in the top-level directory. + +from avocado_qemu import Test +from subprocess import Popen, PIPE + +MAX_NODES = 128 + +class HostNodesValidation(Test): + def test_large_host_nodes(self): + p = Popen([self.qemu_bin, '-display', 'none', '-nodefaults', + '-object', 'memory-backend-ram,id=m0,' + 'size=4096,host-nodes=%d' % (MAX_NODES)], + stderr=PIPE, stdout=PIPE) + stdout,stderr = p.communicate() + + self.assertIn(b'Invalid host-nodes', stderr) + self.assertEquals(stdout, b'') + self.assertEquals(p.returncode, 1) + + def test_valid_host_nodes(self): + p = Popen([self.qemu_bin, '-display', 'none', '-nodefaults', + '-object', 'memory-backend-ram,id=m0,' + 'size=4096,host-nodes=%d' % (MAX_NODES - 1)], + stderr=PIPE, stdout=PIPE) + stdout,stderr = p.communicate() + + self.assertIn(b'host-nodes must be empty', stderr) + self.assertEquals(p.returncode, 1)
Signature · 1 line
2.18.0.rc1.1.g3f1ff2140
Quoted 30 lines · Eduardo Habkost
host_memory_backend_set_host_nodes()was not validating host-nodes before writing tobackend->host_nodes, making QEMU write beyond the end of the bitmap.Fix the crash and add a simple regression test for the fix.
Reported-by Markus Armbruster <[email protected]>Signed-off-by Eduardo Habkost <[email protected]>--- backends/hostmem.c | 13 +++++++--- tests/acceptance/host-nodes-limit.py | 36 ++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) create mode 100644 tests/acceptance/host-nodes-limit.pybackends/hostmem.c +5
-2@@ -103,11 +103,18 @@ 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); + +for (l = host_nodes; l; l = l->next) { +if (l->value >=MAX_NODES ) {
backends/hostmem.c host_memory_backend_set_host_nodes(Object *obj, Visitor *v, const char *name, · line 120
Quoted 10 lines
backends/hostmem.c +5
-1+error_setg (errp,"Invalid host-nodes value: %d" , l->value); +return ; + } + } -while (l) { +for (l = host_nodes; l; l = l->next) {bitmap_set (backend->host_nodes, l->value,1 ); l = l->next; }
Pre-existing: leaks the list created by visit_type_uint16List(), or am I confused?
host_memory_backend_set_host_nodes() looks like it leaks, too.
The function to free such lists is qapi_free_uint16List().
Quoted 32 lines
tests/acceptance/host-nodes-limit.py new file +26
-0@@ -0,0 +1,36 @@ +# Regression test for host-nodes limit validation +# +# Copyright (c) 2018 Red Hat, Inc. +# +# Author: +# Eduardo Habkost <[email protected]> +# +# This work is licensed under the terms of the GNU GPL, version 2 or +# later. See the COPYING file in the top-level directory. + +from avocado_qemu import Test +from subprocess import Popen, PIPE + +MAX_NODES = 128 + +class HostNodesValidation(Test): + def test_large_host_nodes(self): + p = Popen([self.qemu_bin, '-display', 'none', '-nodefaults', + '-object', 'memory-backend-ram,id=m0,' + 'size=4096,host-nodes=%d' % (MAX_NODES)], + stderr=PIPE, stdout=PIPE) + stdout,stderr = p.communicate() + + self.assertIn(b'Invalid host-nodes', stderr) + self.assertEquals(stdout, b'') + self.assertEquals(p.returncode, 1)
tests/acceptance/host-nodes-limit.py line 36
Quoted 10 lines
tests/acceptance/host-nodes-limit.py new file +10
-0+ + def test_valid_host_nodes(self): + p = Popen([self.qemu_bin, '-display', 'none', '-nodefaults', + '-object', 'memory-backend-ram,id=m0,' + 'size=4096,host-nodes=%d' % (MAX_NODES - 1)], + stderr=PIPE, stdout=PIPE) + stdout,stderr = p.communicate() + + self.assertIn(b'host-nodes must be empty', stderr) + self.assertEquals(p.returncode, 1)
Perhaps this test should become part of a more comprehensive menory-backend-ram test.
Quoted 37 lines · Markus Armbruster
Quoted 35 lines · Eduardo Habkost
host_memory_backend_set_host_nodes()was not validating host-nodes before writing tobackend->host_nodes, making QEMU write beyond the end of the bitmap.Fix the crash and add a simple regression test for the fix.
Reported-by Markus Armbruster <[email protected]>Signed-off-by Eduardo Habkost <[email protected]>--- backends/hostmem.c | 13 +++++++--- tests/acceptance/host-nodes-limit.py | 36 ++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) create mode 100644 tests/acceptance/host-nodes-limit.pybackends/hostmem.c +9
-2@@ -103,11 +103,18 @@ 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); + +for (l = host_nodes; l; l = l->next) { +if (l->value >=MAX_NODES ) { +error_setg (errp,"Invalid host-nodes value: %d" , l->value); +return ; + } + }
Quoted 9 lines
backends/hostmem.c host_memory_backend_set_host_nodes(Object *obj, Visitor *v, const char *name, · line 120
Quoted 5 lines
backends/hostmem.c +1
-1-while (l) { +for (l = host_nodes; l; l = l->next) {bitmap_set (backend->host_nodes, l->value,1 ); l = l->next; }Pre-existing: leaks the list created by
visit_type_uint16List(), or am I confused?
I think you're right.
Signature · 3 lines
Thanks, David / dhildenb
Quoted 46 lines · Markus Armbruster
Quoted 30 lines · Eduardo Habkost
host_memory_backend_set_host_nodes()was not validating host-nodes before writing tobackend->host_nodes, making QEMU write beyond the end of the bitmap.Fix the crash and add a simple regression test for the fix.
Reported-by Markus Armbruster <[email protected]>Signed-off-by Eduardo Habkost <[email protected]>--- backends/hostmem.c | 13 +++++++--- tests/acceptance/host-nodes-limit.py | 36 ++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) create mode 100644 tests/acceptance/host-nodes-limit.pybackends/hostmem.c +5
-2@@ -103,11 +103,18 @@ 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); + +for (l = host_nodes; l; l = l->next) { +if (l->value >=MAX_NODES ) {backends/hostmem.c host_memory_backend_set_host_nodes(Object *obj, Visitor *v, const char *name, · line 120
Quoted 10 lines
backends/hostmem.c +5
-1+error_setg (errp,"Invalid host-nodes value: %d" , l->value); +return ; + } + } -while (l) { +for (l = host_nodes; l; l = l->next) {bitmap_set (backend->host_nodes, l->value,1 ); l = l->next; }Pre-existing: leaks the list created by
visit_type_uint16List(), or am I confused?
Quoted 3 lines
host_memory_backend_set_host_nodes()looks like it leaks, too.The function to free such lists is
qapi_free_uint16List().
Thanks, I will fix it in v2.
Quoted 38 lines
Quoted 38 lines
tests/acceptance/host-nodes-limit.py new file +31
-0@@ -0,0 +1,36 @@ +# Regression test for host-nodes limit validation +# +# Copyright (c) 2018 Red Hat, Inc. +# +# Author: +# Eduardo Habkost <[email protected]> +# +# This work is licensed under the terms of the GNU GPL, version 2 or +# later. See the COPYING file in the top-level directory. + +from avocado_qemu import Test +from subprocess import Popen, PIPE + +MAX_NODES = 128 + +class HostNodesValidation(Test): + def test_large_host_nodes(self): + p = Popen([self.qemu_bin, '-display', 'none', '-nodefaults', + '-object', 'memory-backend-ram,id=m0,' + 'size=4096,host-nodes=%d' % (MAX_NODES)], + stderr=PIPE, stdout=PIPE) + stdout,stderr = p.communicate() + + self.assertIn(b'Invalid host-nodes', stderr) + self.assertEquals(stdout, b'') + self.assertEquals(p.returncode, 1) + + def test_valid_host_nodes(self): + p = Popen([self.qemu_bin, '-display', 'none', '-nodefaults', + '-object', 'memory-backend-ram,id=m0,' + 'size=4096,host-nodes=%d' % (MAX_NODES - 1)],
Quoted 8 lines
tests/acceptance/host-nodes-limit.py line 36
Quoted 5 lines
tests/acceptance/host-nodes-limit.py new file +5
-0+ stderr=PIPE, stdout=PIPE) + stdout,stderr = p.communicate() + + self.assertIn(b'host-nodes must be empty', stderr) + self.assertEquals(p.returncode, 1)Perhaps this test should become part of a more comprehensive menory-backend-ram test.
If such a test existed, yes.
Quoted 2 lines
Reviewed-by Markus Armbruster <[email protected]>
Thanks!
(Although this version is broken due to the extra "l = l->next" line and we need v2)
Signature · 1 line
Eduardo
Quoted 31 lines · Eduardo Habkost
host_memory_backend_set_host_nodes()was not validating host-nodes before writing tobackend->host_nodes, making QEMU write beyond the end of the bitmap.Fix the crash and add a simple regression test for the fix.
Reported-by Markus Armbruster <[email protected]>Signed-off-by Eduardo Habkost <[email protected]>--- backends/hostmem.c | 13 +++++++--- tests/acceptance/host-nodes-limit.py | 36 ++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) create mode 100644 tests/acceptance/host-nodes-limit.pybackends/hostmem.c +5
-2@@ -103,11 +103,18 @@ 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); + +for (l = host_nodes; l; l = l->next) { +if (l->value >=MAX_NODES ) {
backends/hostmem.c host_memory_backend_set_host_nodes(Object *obj, Visitor *v, const char *name, · line 120
Quoted 10 lines
backends/hostmem.c +5
-1+error_setg (errp,"Invalid host-nodes value: %d" , l->value); +return ; + } + } -while (l) { +for (l = host_nodes; l; l = l->next) {bitmap_set (backend->host_nodes, l->value,1 ); l = l->next; }
Using the for loop instead of the while, maybe we need to remove the l = l->next in the for body.
Cheers,
Stefano
Quoted 43 lines
tests/acceptance/host-nodes-limit.py new file +36
-1@@ -0,0 +1,36 @@ +# Regression test for host-nodes limit validation +# +# Copyright (c) 2018 Red Hat, Inc. +# +# Author: +# Eduardo Habkost <[email protected]> +# +# This work is licensed under the terms of the GNU GPL, version 2 or +# later. See the COPYING file in the top-level directory. + +from avocado_qemu import Test +from subprocess import Popen, PIPE + +MAX_NODES = 128 + +class HostNodesValidation(Test): + def test_large_host_nodes(self): + p = Popen([self.qemu_bin, '-display', 'none', '-nodefaults', + '-object', 'memory-backend-ram,id=m0,' + 'size=4096,host-nodes=%d' % (MAX_NODES)], + stderr=PIPE, stdout=PIPE) + stdout,stderr = p.communicate() + + self.assertIn(b'Invalid host-nodes', stderr) + self.assertEquals(stdout, b'') + self.assertEquals(p.returncode, 1) + + def test_valid_host_nodes(self): + p = Popen([self.qemu_bin, '-display', 'none', '-nodefaults', + '-object', 'memory-backend-ram,id=m0,' + 'size=4096,host-nodes=%d' % (MAX_NODES - 1)], + stderr=PIPE, stdout=PIPE) + stdout,stderr = p.communicate() + + self.assertIn(b'host-nodes must be empty', stderr) + self.assertEquals(p.returncode, 1) --
Quoted 3 lines
2.18.0.rc1.1.g3f1ff2140
--
Stefano Garzarella
Red Hat
Quoted 37 lines · Stefano Garzarella
Quoted 36 lines · Eduardo Habkost
host_memory_backend_set_host_nodes()was not validating host-nodes before writing tobackend->host_nodes, making QEMU write beyond the end of the bitmap.Fix the crash and add a simple regression test for the fix.
Reported-by Markus Armbruster <[email protected]>Signed-off-by Eduardo Habkost <[email protected]>--- backends/hostmem.c | 13 +++++++--- tests/acceptance/host-nodes-limit.py | 36 ++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) create mode 100644 tests/acceptance/host-nodes-limit.pybackends/hostmem.c +9
-2@@ -103,11 +103,18 @@ 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); + +for (l = host_nodes; l; l = l->next) { +if (l->value >=MAX_NODES ) { +error_setg (errp,"Invalid host-nodes value: %d" , l->value); +return ; + } + }
Quoted 8 lines
backends/hostmem.c host_memory_backend_set_host_nodes(Object *obj, Visitor *v, const char *name, · line 120
Quoted 5 lines
backends/hostmem.c +1
-1-while (l) { +for (l = host_nodes; l; l = l->next) {bitmap_set (backend->host_nodes, l->value,1 ); l = l->next; }Using the for loop instead of the while, maybe we need to remove the l =
l->nextin the for body.
Yeah, we must remove it. I did remove it locally but I think I forgot to amend the commit before submitting. Thanks for catching!
Signature · 1 line
Eduardo
Quoted 25 lines · Eduardo Habkost
host_memory_backend_set_host_nodes()was not validating host-nodes before writing tobackend->host_nodes, making QEMU write beyond the end of the bitmap.Fix the crash and add a simple regression test for the fix.
Reported-by Markus Armbruster <[email protected]>Signed-off-by Eduardo Habkost <[email protected]>--- backends/hostmem.c | 13 +++++++--- tests/acceptance/host-nodes-limit.py | 36 ++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) create mode 100644 tests/acceptance/host-nodes-limit.pybackends/hostmem.c +1
-1@@ -103,11 +103,18 @@ 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 ;
backends/hostmem.c host_memory_backend_set_host_nodes(Object *obj, Visitor *v, const char *name, · line 116
Quoted 10 lines
backends/hostmem.c +8
-1-visit_type_uint16List (v, name, &l, errp); +visit_type_uint16List (v, name, &host_nodes, errp); + +for (l = host_nodes; l; l = l->next) { +if (l->value >=MAX_NODES ) { +error_setg (errp,"Invalid host-nodes value: %d" , l->value); +return ; + } + }
An alternative would be to use a temporary bitmap (on the stack) instead and copy it only on success. E.g.
for (l = host_nodes; l; l = l->next) {
if (l->value >= MAX_NODE) {
error_setg(&local_err, "Invalid host-nodes value: %d", l->value);
break;
}
bitmap_set(tmp, l->value, 1);
}if (!local_err) {
bitmap_copy(backend->host_nodes, tmp, MAX_NODE);
}error_propagate...
free...
Quoted 48 lines
backends/hostmem.c +1
-1-while (l) { +for (l = host_nodes; l; l = l->next) {bitmap_set (backend->host_nodes, l->value,1 ); l = l->next; }tests/acceptance/host-nodes-limit.py new file +36
-0@@ -0,0 +1,36 @@ +# Regression test for host-nodes limit validation +# +# Copyright (c) 2018 Red Hat, Inc. +# +# Author: +# Eduardo Habkost <[email protected]> +# +# This work is licensed under the terms of the GNU GPL, version 2 or +# later. See the COPYING file in the top-level directory. + +from avocado_qemu import Test +from subprocess import Popen, PIPE + +MAX_NODES = 128 + +class HostNodesValidation(Test): + def test_large_host_nodes(self): + p = Popen([self.qemu_bin, '-display', 'none', '-nodefaults', + '-object', 'memory-backend-ram,id=m0,' + 'size=4096,host-nodes=%d' % (MAX_NODES)], + stderr=PIPE, stdout=PIPE) + stdout,stderr = p.communicate() + + self.assertIn(b'Invalid host-nodes', stderr) + self.assertEquals(stdout, b'') + self.assertEquals(p.returncode, 1) + + def test_valid_host_nodes(self): + p = Popen([self.qemu_bin, '-display', 'none', '-nodefaults', + '-object', 'memory-backend-ram,id=m0,' + 'size=4096,host-nodes=%d' % (MAX_NODES - 1)], + stderr=PIPE, stdout=PIPE) + stdout,stderr = p.communicate() + + self.assertIn(b'host-nodes must be empty', stderr) + self.assertEquals(p.returncode, 1)
Signature · 3 lines
Thanks, David / dhildenb