[libvirt] [PATCH] qemu: Avoid crashes in qemuDomainMachineIs*()
Make sure pointers are non-NULL before dereferencing them, and add test suite coverage for the crashers doing so fixes. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1379218 Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 35 ++++++++-------- .../qemuargv2xml-nomachine-aarch64.args | 11 +++++ .../qemuargv2xml-nomachine-aarch64.xml | 39 ++++++++++++++++++ .../qemuargv2xml-nomachine-ppc64.args | 11 +++++ .../qemuargv2xml-nomachine-ppc64.xml | 47 +++++++++++++++++++++ .../qemuargv2xml-nomachine-x86_64.args | 11 +++++ .../qemuargv2xml-nomachine-x86_64.xml | 48 ++++++++++++++++++++++ tests/qemuargv2xmltest.c | 4 ++ 8 files changed, 188 insertions(+), 18 deletions(-) create mode 100644 tests/qemuargv2xmldata/qemuargv2xml-nomachine-aarch64.args create mode 100644 tests/qemuargv2xmldata/qemuargv2xml-nomachine-aarch64.xml create mode 100644 tests/qemuargv2xmldata/qemuargv2xml-nomachine-ppc64.args create mode 100644 tests/qemuargv2xmldata/qemuargv2xml-nomachine-ppc64.xml create mode 100644 tests/qemuargv2xmldata/qemuargv2xml-nomachine-x86_64.args create mode 100644 tests/qemuargv2xmldata/qemuargv2xml-nomachine-x86_64.xml diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b202d02f9..30ea3e592 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6601,8 +6601,9 @@ qemuDomainIsQ35(const virDomainDef *def) bool qemuDomainMachineIsQ35(const char *machine) { - return (STRPREFIX(machine, "pc-q35") || - STREQ(machine, "q35")); + return (machine && + (STREQ(machine, "q35") || + STRPREFIX(machine, "pc-q35"))); } @@ -6616,11 +6617,12 @@ qemuDomainIsI440FX(const virDomainDef *def) bool qemuDomainMachineIsI440FX(const char *machine) { - return (STREQ(machine, "pc") || - STRPREFIX(machine, "pc-0.") || - STRPREFIX(machine, "pc-1.") || - STRPREFIX(machine, "pc-i440") || - STRPREFIX(machine, "rhel")); + return (machine && + (STREQ(machine, "pc") || + STRPREFIX(machine, "pc-0.") || + STRPREFIX(machine, "pc-1.") || + STRPREFIX(machine, "pc-i440") || + STRPREFIX(machine, "rhel"))); } @@ -6689,7 +6691,8 @@ qemuDomainIsS390CCW(const virDomainDef *def) bool qemuDomainMachineIsS390CCW(const char *machine) { - return STRPREFIX(machine, "s390-ccw"); + return (machine && + STRPREFIX(machine, "s390-ccw")); } @@ -6708,11 +6711,9 @@ qemuDomainMachineIsVirt(const char *machine, arch != VIR_ARCH_AARCH64) return false; - if (STRNEQ(machine, "virt") && - !STRPREFIX(machine, "virt-")) - return false; - - return true; + return (machine && + (STREQ(machine, "virt") || + STRPREFIX(machine, "virt-"))); } @@ -6730,11 +6731,9 @@ qemuDomainMachineIsPSeries(const char *machine, if (!ARCH_IS_PPC64(arch)) return false; - if (STRNEQ(machine, "pseries") && - !STRPREFIX(machine, "pseries-")) - return false; - - return true; + return (machine && + (STREQ(machine, "pseries") || + STRPREFIX(machine, "pseries-"))); } diff --git a/tests/qemuargv2xmldata/qemuargv2xml-nomachine-aarch64.args b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-aarch64.args new file mode 100644 index 000000000..b17c0d0c2 --- /dev/null +++ b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-aarch64.args @@ -0,0 +1,11 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-aarch64 \ +-name QEMUGuest1 \ +-m 512 \ +-hda /dev/HostVG/QEMUGuest1 \ +-cdrom /root/boot.iso diff --git a/tests/qemuargv2xmldata/qemuargv2xml-nomachine-aarch64.xml b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-aarch64.xml new file mode 100644 index 000000000..eb8f9db80 --- /dev/null +++ b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-aarch64.xml @@ -0,0 +1,39 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>524288</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='aarch64' machine='virt'>hvm</type> + </os> + <features> + <gic version='2'/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-aarch64</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='file' device='cdrom'> + <driver name='qemu' type='raw'/> + <source file='/root/boot.iso'/> + <target dev='hdc' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='1' target='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <graphics type='sdl'/> + <video> + <model type='cirrus' vram='16384' heads='1' primary='yes'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuargv2xmldata/qemuargv2xml-nomachine-ppc64.args b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-ppc64.args new file mode 100644 index 000000000..ac618775e --- /dev/null +++ b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-ppc64.args @@ -0,0 +1,11 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-ppc64 \ +-name QEMUGuest1 \ +-m 512 \ +-hda /dev/HostVG/QEMUGuest1 \ +-cdrom /root/boot.iso diff --git a/tests/qemuargv2xmldata/qemuargv2xml-nomachine-ppc64.xml b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-ppc64.xml new file mode 100644 index 000000000..1e987f645 --- /dev/null +++ b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-ppc64.xml @@ -0,0 +1,47 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>524288</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='file' device='cdrom'> + <driver name='qemu' type='raw'/> + <source file='/root/boot.iso'/> + <target dev='hdc' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='1' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> + <controller type='ide' index='0'/> + <input type='keyboard' bus='usb'/> + <input type='mouse' bus='usb'/> + <graphics type='sdl'/> + <video> + <model type='cirrus' vram='16384' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <memballoon model='none'/> + <panic model='pseries'/> + </devices> +</domain> diff --git a/tests/qemuargv2xmldata/qemuargv2xml-nomachine-x86_64.args b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-x86_64.args new file mode 100644 index 000000000..22bc4fb1c --- /dev/null +++ b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-x86_64.args @@ -0,0 +1,11 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name QEMUGuest1 \ +-m 512 \ +-hda /dev/HostVG/QEMUGuest1 \ +-cdrom /root/boot.iso diff --git a/tests/qemuargv2xmldata/qemuargv2xml-nomachine-x86_64.xml b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-x86_64.xml new file mode 100644 index 000000000..71a36f083 --- /dev/null +++ b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-x86_64.xml @@ -0,0 +1,48 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>524288</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-0.11'>hvm</type> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='file' device='cdrom'> + <driver name='qemu' type='raw'/> + <source file='/root/boot.iso'/> + <target dev='hdc' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='1' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='sdl'/> + <video> + <model type='cirrus' vram='16384' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 1adbcfef6..5d261afa6 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -288,6 +288,10 @@ mymain(void) DO_TEST("machine-deakeywrap-off-argv"); DO_TEST("machine-keywrap-none-argv"); + DO_TEST("nomachine-x86_64"); + DO_TEST("nomachine-aarch64"); + DO_TEST("nomachine-ppc64"); + qemuTestDriverFree(&driver); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; -- 2.13.6
On Tue, Oct 10, 2017 at 04:21:02PM +0200, Andrea Bolognani wrote:
Make sure pointers are non-NULL before dereferencing them, and add test suite coverage for the crashers doing so fixes.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1379218
Hmm, I don't think that is a good fix for the problem seen there. We're parsing the CLI argv from an existing QEMU process and when looking at the disk we're checking if the machine type refers to a pseries guest. The problem is that either the user might not have given any -machine arg, or the -drive arg might occur *before* the -machine arg is parsed. Simply making the qemuDomainMachineIs* safe against NULL will avoid the crash, but the ARGV parsing is still going to be semantically broken. As a more general point, we've tended to assume that machine is always non-NULL throughout the code, because we rely on the XML parsing to fill in defaults if omitted by the user. I think rather than trying to fix up the assumption about machine being non-NULL, we should restructure the ARGV parsing into we need a 2 pass process. In the first pass only look for the -machine arg. If no -machine arg is given, we should fill in the default machine for that emulator. In the second pass process the rest of the ARGV, whereupon we have a valid assumption that machine is non-NULL. 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 :|
On Tue, 2017-10-10 at 15:49 +0100, Daniel P. Berrange wrote:
I think rather than trying to fix up the assumption about machine being non-NULL, we should restructure the ARGV parsing into we need a 2 pass process.
In the first pass only look for the -machine arg. If no -machine arg is given, we should fill in the default machine for that emulator.
I can do this...
In the second pass process the rest of the ARGV, whereupon we have a valid assumption that machine is non-NULL.
... but I'm not sure doing this is a good idea. Wouldn't it be much safer to leave the newly-added NULL checks in place so that the qemuDomainMachineIs*() functions are locally correct instead of relying on external guarantees in order not to crash on the user? -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Oct 10, 2017 at 05:05:33PM +0200, Andrea Bolognani wrote:
On Tue, 2017-10-10 at 15:49 +0100, Daniel P. Berrange wrote:
I think rather than trying to fix up the assumption about machine being non-NULL, we should restructure the ARGV parsing into we need a 2 pass process.
In the first pass only look for the -machine arg. If no -machine arg is given, we should fill in the default machine for that emulator.
I can do this...
In the second pass process the rest of the ARGV, whereupon we have a valid assumption that machine is non-NULL.
... but I'm not sure doing this is a good idea.
Wouldn't it be much safer to leave the newly-added NULL checks in place so that the qemuDomainMachineIs*() functions are locally correct instead of relying on external guarantees in order not to crash on the user?
Are these qemuDomainMachineIs* functions the only places that makes assumptions about machine being non-NULL though ? I wouldn't want to encourage the idea that it is OK to have a NULL machine value, because that would just push crashes to elsewhere in the code if people rely on it. 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 :|
On Tue, 2017-10-10 at 16:10 +0100, Daniel P. Berrange wrote:
Wouldn't it be much safer to leave the newly-added NULL checks in place so that the qemuDomainMachineIs*() functions are locally correct instead of relying on external guarantees in order not to crash on the user?
Are these qemuDomainMachineIs* functions the only places that makes assumptions about machine being non-NULL though ?
I guess we don't know. And that's why assumptions should be verified locally :)
I wouldn't want to encourage the idea that it is OK to have a NULL machine value, because that would just push crashes to elsewhere in the code if people rely on it.
Okay, I'll drop those hunks from the respin then. -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Daniel P. Berrange