[PATCH 0/2] libxl_conf: Fix crashes in libxl config generation
This patch series includes fixes for config generation of multiple serial devices and handling of unsupported graphics types. Both of these were discovered some time back using fuzzing techniques. Rayhan Faizel (2): libxl_conf: Fix config generation for multiple serial devices libxl_conf: Add check for unsupported graphics type src/libxl/libxl_conf.c | 15 +++-- .../multiple-serial.json | 63 +++++++++++++++++++ .../multiple-serial.xml | 47 ++++++++++++++ .../libxlxml2domconfigdata/single-serial.json | 52 +++++++++++++++ .../libxlxml2domconfigdata/single-serial.xml | 25 ++++++++ tests/libxlxml2domconfigtest.c | 3 + 6 files changed, 200 insertions(+), 5 deletions(-) create mode 100644 tests/libxlxml2domconfigdata/multiple-serial.json create mode 100644 tests/libxlxml2domconfigdata/multiple-serial.xml create mode 100644 tests/libxlxml2domconfigdata/single-serial.json create mode 100644 tests/libxlxml2domconfigdata/single-serial.xml -- 2.34.1
Currently, an array of libxl_string_list (char **) or in other words, a triple char pointer is initialized. This is dereferenced to a char ** type and stored in serial_list, which is NULL at this point. There is an attempt to reference an element of this serial_list when making a call to libxlMakeChrdevStr which causes a segmentation fault. To fix this, we simply allocate an array of char * instead of libxl_string_list. This patch also adds testcases to extend coverage over both single serial and multiple serial cases. Signed-off-by: Rayhan Faizel <rayhan.faizel@gmail.com> --- src/libxl/libxl_conf.c | 2 +- .../multiple-serial.json | 63 +++++++++++++++++++ .../multiple-serial.xml | 47 ++++++++++++++ .../libxlxml2domconfigdata/single-serial.json | 52 +++++++++++++++ .../libxlxml2domconfigdata/single-serial.xml | 25 ++++++++ tests/libxlxml2domconfigtest.c | 3 + 6 files changed, 191 insertions(+), 1 deletion(-) create mode 100644 tests/libxlxml2domconfigdata/multiple-serial.json create mode 100644 tests/libxlxml2domconfigdata/multiple-serial.xml create mode 100644 tests/libxlxml2domconfigdata/single-serial.json create mode 100644 tests/libxlxml2domconfigdata/single-serial.xml diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 62e1be6672..8c91489ffd 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -692,7 +692,7 @@ libxlMakeDomBuildInfo(virDomainDef *def, 0) return -1; } else { - b_info->u.hvm.serial_list = *g_new0(libxl_string_list, def->nserials + 1); + b_info->u.hvm.serial_list = g_new0(char *, def->nserials + 1); for (i = 0; i < def->nserials; i++) { if (libxlMakeChrdevStr(def->serials[i], &b_info->u.hvm.serial_list[i]) < 0) diff --git a/tests/libxlxml2domconfigdata/multiple-serial.json b/tests/libxlxml2domconfigdata/multiple-serial.json new file mode 100644 index 0000000000..121f2d1260 --- /dev/null +++ b/tests/libxlxml2domconfigdata/multiple-serial.json @@ -0,0 +1,63 @@ +{ + "c_info": { + "type": "hvm", + "name": "test-hvm", + "uuid": "2147d599-9cc6-c0dc-92ab-4064b5446e9b" + }, + "b_info": { + "max_vcpus": 4, + "avail_vcpus": [ + 0, + 1, + 2, + 3 + ], + "max_memkb": 1048576, + "target_memkb": 1048576, + "shadow_memkb": 1234, + "sched_params": { + + }, + "acpi": "True", + "apic": "True", + "type.hvm": { + "pae": "True", + "nographic": "True", + "vga": { + "kind": "none" + }, + "vnc": { + "enable": "False" + }, + "sdl": { + "enable": "False" + }, + "spice": { + + }, + "serial_list": [ + "null", + "stdio", + "vc", + "pty", + "pipe:/tmp/file", + "file:/tmp/serial.log", + "/dev/ttyS2", + "udp::9999@:0", + "tcp:127.0.0.1:9999", + "unix:/tmp/serial-server.sock,server,nowait" + ], + "boot": "c", + "rdm": { + + } + }, + "arch_arm": { + + }, + "arch_x86": { + + } + }, + "on_reboot": "restart" +} diff --git a/tests/libxlxml2domconfigdata/multiple-serial.xml b/tests/libxlxml2domconfigdata/multiple-serial.xml new file mode 100644 index 0000000000..c50ffd0bb4 --- /dev/null +++ b/tests/libxlxml2domconfigdata/multiple-serial.xml @@ -0,0 +1,47 @@ +<domain type='xen'> + <name>test-hvm</name> + <description>None</description> + <uuid>2147d599-9cc6-c0dc-92ab-4064b5446e9b</uuid> + <memory>1048576</memory> + <currentMemory>1048576</currentMemory> + <vcpu>4</vcpu> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <clock offset='utc'/> + <os> + <type>hvm</type> + <loader>/usr/lib/xen/boot/hvmloader</loader> + <boot dev='hd'/> + </os> + <features> + <apic/> + <acpi/> + <pae/> + </features> + <devices> + <serial type='null'/> + <serial type='stdio'/> + <serial type='vc'/> + <serial type='pty'/> + <serial type='pipe'> + <source path='/tmp/file'/> + </serial> + <serial type='file'> + <source path='/tmp/serial.log'/> + </serial> + <serial type='dev'> + <source path='/dev/ttyS2'/> + </serial> + <serial type='udp'> + <source mode='connect' service='9999'/> + </serial> + <serial type='tcp'> + <source mode='connect' host='127.0.0.1' service='9999'/> + <protocol type='raw'/> + </serial> + <serial type='unix'> + <source mode='bind' path='/tmp/serial-server.sock'/> + </serial> + </devices> +</domain> diff --git a/tests/libxlxml2domconfigdata/single-serial.json b/tests/libxlxml2domconfigdata/single-serial.json new file mode 100644 index 0000000000..a736e6f805 --- /dev/null +++ b/tests/libxlxml2domconfigdata/single-serial.json @@ -0,0 +1,52 @@ +{ + "c_info": { + "type": "hvm", + "name": "test-hvm", + "uuid": "2147d599-9cc6-c0dc-92ab-4064b5446e9b" + }, + "b_info": { + "max_vcpus": 4, + "avail_vcpus": [ + 0, + 1, + 2, + 3 + ], + "max_memkb": 1048576, + "target_memkb": 1048576, + "shadow_memkb": 1234, + "sched_params": { + + }, + "acpi": "True", + "apic": "True", + "type.hvm": { + "pae": "True", + "nographic": "True", + "vga": { + "kind": "none" + }, + "vnc": { + "enable": "False" + }, + "sdl": { + "enable": "False" + }, + "spice": { + + }, + "serial": "pty", + "boot": "c", + "rdm": { + + } + }, + "arch_arm": { + + }, + "arch_x86": { + + } + }, + "on_reboot": "restart" +} diff --git a/tests/libxlxml2domconfigdata/single-serial.xml b/tests/libxlxml2domconfigdata/single-serial.xml new file mode 100644 index 0000000000..f468024189 --- /dev/null +++ b/tests/libxlxml2domconfigdata/single-serial.xml @@ -0,0 +1,25 @@ +<domain type='xen'> + <name>test-hvm</name> + <description>None</description> + <uuid>2147d599-9cc6-c0dc-92ab-4064b5446e9b</uuid> + <memory>1048576</memory> + <currentMemory>1048576</currentMemory> + <vcpu>4</vcpu> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <clock offset='utc'/> + <os> + <type>hvm</type> + <loader>/usr/lib/xen/boot/hvmloader</loader> + <boot dev='hd'/> + </os> + <features> + <apic/> + <acpi/> + <pae/> + </features> + <devices> + <serial type='pty'/> + </devices> +</domain> diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c index 21c4e7d149..255855b156 100644 --- a/tests/libxlxml2domconfigtest.c +++ b/tests/libxlxml2domconfigtest.c @@ -208,6 +208,9 @@ mymain(void) DO_TEST("max-eventchannels-hvm"); + DO_TEST("single-serial"); + DO_TEST("multiple-serial"); + unlink("libxl-driver.log"); testXLFreeDriver(driver); -- 2.34.1
On Tue, Sep 17, 2024 at 11:28:45PM +0530, Rayhan Faizel wrote:
Currently, an array of libxl_string_list (char **) or in other words, a triple char pointer is initialized. This is dereferenced to a char ** type and stored in serial_list, which is NULL at this point. There is an attempt to reference an element of this serial_list when making a call to libxlMakeChrdevStr which causes a segmentation fault.
To fix this, we simply allocate an array of char * instead of libxl_string_list.
This patch also adds testcases to extend coverage over both single serial and multiple serial cases.
Signed-off-by: Rayhan Faizel <rayhan.faizel@gmail.com> --- src/libxl/libxl_conf.c | 2 +- .../multiple-serial.json | 63 +++++++++++++++++++ .../multiple-serial.xml | 47 ++++++++++++++ .../libxlxml2domconfigdata/single-serial.json | 52 +++++++++++++++ .../libxlxml2domconfigdata/single-serial.xml | 25 ++++++++ tests/libxlxml2domconfigtest.c | 3 + 6 files changed, 191 insertions(+), 1 deletion(-) create mode 100644 tests/libxlxml2domconfigdata/multiple-serial.json create mode 100644 tests/libxlxml2domconfigdata/multiple-serial.xml create mode 100644 tests/libxlxml2domconfigdata/single-serial.json create mode 100644 tests/libxlxml2domconfigdata/single-serial.xml
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 62e1be6672..8c91489ffd 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -692,7 +692,7 @@ libxlMakeDomBuildInfo(virDomainDef *def, 0) return -1; } else { - b_info->u.hvm.serial_list = *g_new0(libxl_string_list, def->nserials + 1); + b_info->u.hvm.serial_list = g_new0(char *, def->nserials + 1); for (i = 0; i < def->nserials; i++) { if (libxlMakeChrdevStr(def->serials[i], &b_info->u.hvm.serial_list[i]) < 0)
right below this line, in case of an error, is a call to: libxl_string_list_dispose(&b_info->u.hvm.serial_list); but since we are the ones constructing it I think it would be safer to also dispose of it with our functions as I could not find what and how that libxl_ function does that. Anyway, this is strictly better than before, so the adjustment can be done in a separate patch. So both of these are Reviewed-by: Martin Kletzander <mkletzan@redhat.com> and I'll push them in a while. Thanks for the patches.
diff --git a/tests/libxlxml2domconfigdata/multiple-serial.json b/tests/libxlxml2domconfigdata/multiple-serial.json new file mode 100644 index 0000000000..121f2d1260 --- /dev/null +++ b/tests/libxlxml2domconfigdata/multiple-serial.json @@ -0,0 +1,63 @@ +{ + "c_info": { + "type": "hvm", + "name": "test-hvm", + "uuid": "2147d599-9cc6-c0dc-92ab-4064b5446e9b" + }, + "b_info": { + "max_vcpus": 4, + "avail_vcpus": [ + 0, + 1, + 2, + 3 + ], + "max_memkb": 1048576, + "target_memkb": 1048576, + "shadow_memkb": 1234, + "sched_params": { + + }, + "acpi": "True", + "apic": "True", + "type.hvm": { + "pae": "True", + "nographic": "True", + "vga": { + "kind": "none" + }, + "vnc": { + "enable": "False" + }, + "sdl": { + "enable": "False" + }, + "spice": { + + }, + "serial_list": [ + "null", + "stdio", + "vc", + "pty", + "pipe:/tmp/file", + "file:/tmp/serial.log", + "/dev/ttyS2", + "udp::9999@:0", + "tcp:127.0.0.1:9999", + "unix:/tmp/serial-server.sock,server,nowait" + ], + "boot": "c", + "rdm": { + + } + }, + "arch_arm": { + + }, + "arch_x86": { + + } + }, + "on_reboot": "restart" +} diff --git a/tests/libxlxml2domconfigdata/multiple-serial.xml b/tests/libxlxml2domconfigdata/multiple-serial.xml new file mode 100644 index 0000000000..c50ffd0bb4 --- /dev/null +++ b/tests/libxlxml2domconfigdata/multiple-serial.xml @@ -0,0 +1,47 @@ +<domain type='xen'> + <name>test-hvm</name> + <description>None</description> + <uuid>2147d599-9cc6-c0dc-92ab-4064b5446e9b</uuid> + <memory>1048576</memory> + <currentMemory>1048576</currentMemory> + <vcpu>4</vcpu> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <clock offset='utc'/> + <os> + <type>hvm</type> + <loader>/usr/lib/xen/boot/hvmloader</loader> + <boot dev='hd'/> + </os> + <features> + <apic/> + <acpi/> + <pae/> + </features> + <devices> + <serial type='null'/> + <serial type='stdio'/> + <serial type='vc'/> + <serial type='pty'/> + <serial type='pipe'> + <source path='/tmp/file'/> + </serial> + <serial type='file'> + <source path='/tmp/serial.log'/> + </serial> + <serial type='dev'> + <source path='/dev/ttyS2'/> + </serial> + <serial type='udp'> + <source mode='connect' service='9999'/> + </serial> + <serial type='tcp'> + <source mode='connect' host='127.0.0.1' service='9999'/> + <protocol type='raw'/> + </serial> + <serial type='unix'> + <source mode='bind' path='/tmp/serial-server.sock'/> + </serial> + </devices> +</domain> diff --git a/tests/libxlxml2domconfigdata/single-serial.json b/tests/libxlxml2domconfigdata/single-serial.json new file mode 100644 index 0000000000..a736e6f805 --- /dev/null +++ b/tests/libxlxml2domconfigdata/single-serial.json @@ -0,0 +1,52 @@ +{ + "c_info": { + "type": "hvm", + "name": "test-hvm", + "uuid": "2147d599-9cc6-c0dc-92ab-4064b5446e9b" + }, + "b_info": { + "max_vcpus": 4, + "avail_vcpus": [ + 0, + 1, + 2, + 3 + ], + "max_memkb": 1048576, + "target_memkb": 1048576, + "shadow_memkb": 1234, + "sched_params": { + + }, + "acpi": "True", + "apic": "True", + "type.hvm": { + "pae": "True", + "nographic": "True", + "vga": { + "kind": "none" + }, + "vnc": { + "enable": "False" + }, + "sdl": { + "enable": "False" + }, + "spice": { + + }, + "serial": "pty", + "boot": "c", + "rdm": { + + } + }, + "arch_arm": { + + }, + "arch_x86": { + + } + }, + "on_reboot": "restart" +} diff --git a/tests/libxlxml2domconfigdata/single-serial.xml b/tests/libxlxml2domconfigdata/single-serial.xml new file mode 100644 index 0000000000..f468024189 --- /dev/null +++ b/tests/libxlxml2domconfigdata/single-serial.xml @@ -0,0 +1,25 @@ +<domain type='xen'> + <name>test-hvm</name> + <description>None</description> + <uuid>2147d599-9cc6-c0dc-92ab-4064b5446e9b</uuid> + <memory>1048576</memory> + <currentMemory>1048576</currentMemory> + <vcpu>4</vcpu> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <clock offset='utc'/> + <os> + <type>hvm</type> + <loader>/usr/lib/xen/boot/hvmloader</loader> + <boot dev='hd'/> + </os> + <features> + <apic/> + <acpi/> + <pae/> + </features> + <devices> + <serial type='pty'/> + </devices> +</domain> diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c index 21c4e7d149..255855b156 100644 --- a/tests/libxlxml2domconfigtest.c +++ b/tests/libxlxml2domconfigtest.c @@ -208,6 +208,9 @@ mymain(void)
DO_TEST("max-eventchannels-hvm");
+ DO_TEST("single-serial"); + DO_TEST("multiple-serial"); + unlink("libxl-driver.log");
testXLFreeDriver(driver); -- 2.34.1
libxlMakeVfb always succeeds regardless of if the graphics type is actually supported or not. libxl_defbool_val is called in libxlMakeBuildInfoVfb which besides returning the boolean value of the defbool also has an assertion that the defbool value is not set to default. It is possible to fail this assertion if an unsupported graphics type is used. In libxlMakeVfb, the VNC and SDL enable defbools are still left in their default state if the graphics type falls outside the two, which leads to this issue. This patch adds a check to reject graphics types outside of SDL, VNC, and SPICE very early on in libxlMakeVfb. As a safeguard, we also initialize both vnc enable and sdl enable defbools as false early. Signed-off-by: Rayhan Faizel <rayhan.faizel@gmail.com> --- src/libxl/libxl_conf.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 8c91489ffd..c404226e43 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1506,18 +1506,18 @@ libxlMakeVfb(virPortAllocatorRange *graphicsports, virDomainGraphicsListenDef *glisten = NULL; libxl_device_vfb_init(x_vfb); + libxl_defbool_set(&x_vfb->sdl.enable, 0); + libxl_defbool_set(&x_vfb->vnc.enable, 0); switch (l_vfb->type) { case VIR_DOMAIN_GRAPHICS_TYPE_SDL: libxl_defbool_set(&x_vfb->sdl.enable, 1); - libxl_defbool_set(&x_vfb->vnc.enable, 0); libxl_defbool_set(&x_vfb->sdl.opengl, 0); x_vfb->sdl.display = g_strdup(l_vfb->data.sdl.display); x_vfb->sdl.xauthority = g_strdup(l_vfb->data.sdl.xauth); break; case VIR_DOMAIN_GRAPHICS_TYPE_VNC: libxl_defbool_set(&x_vfb->vnc.enable, 1); - libxl_defbool_set(&x_vfb->sdl.enable, 0); /* driver handles selection of free port */ libxl_defbool_set(&x_vfb->vnc.findunused, 0); if (l_vfb->data.vnc.autoport) { @@ -1542,13 +1542,18 @@ libxlMakeVfb(virPortAllocatorRange *graphicsports, x_vfb->keymap = g_strdup(l_vfb->data.vnc.keymap); break; + case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: + break; + case VIR_DOMAIN_GRAPHICS_TYPE_RDP: case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP: - case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: case VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS: case VIR_DOMAIN_GRAPHICS_TYPE_DBUS: case VIR_DOMAIN_GRAPHICS_TYPE_LAST: - break; + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported graphics type %1$s"), + virDomainGraphicsTypeToString(l_vfb->type)); + return -1; } return 0; -- 2.34.1
participants (2)
-
Martin Kletzander -
Rayhan Faizel