7:38 a.m.
From: Wim ten Have <wim.ten.have(a)oracle.com>
This patch protects against a NULL pointer dereference leading to a
SEGV and that way taking out libvirtd while performing domain conversion
requests per domxml-from-native xen-xl xl.cfg within xenParseXLDiskSrc()
when partial disk parameters are provided.
Wim ten Have (1):
xenParseXLDiskSrc: protect against a NULL pointer reference
src/xenconfig/xen_xl.c | 3 ++-
tests/xlconfigdata/test-disk-positional-parms-partial.cfg | 2 +-
tests/xlconfigdata/test-disk-positional-parms-partial.xml | 6 ++++++
3 files changed, 9 insertions(+), 2 deletions(-)
--
2.9.4
7:38 a.m.
From: Wim ten Have <wim.ten.have(a)oracle.com>
Working larger code changes whilst testing functionality and domxml
conversion methodology for xen-xl (xenconfig) a cumbersome caveat surfaced
that potentially can take libvirtd out with a SEGV when parsing complex
disk xl.cfg directives.
This patch also includes tests/xlconfigdata adjustments to illustrate
specific disk xl.cfg directive and that way updating test 2-ways.
Running the tests with defensive code fix all will run without trouble,
running it without the code fix testing will trap with below listed
debug transcript.
<wtenhave@nina:21-ba$h> VIR_TEST_DEBUG=1 ./run gdb ./tests/xlconfigtest
TEST: xlconfigtest
1) Xen XL-2-XML Parse fullvirt-ovmf ... OK
2) Xen XL-2-XML Format fullvirt-ovmf ... OK
3) Xen XL-2-XML Parse paravirt-maxvcpus ... OK
4) Xen XL-2-XML Format paravirt-maxvcpus ... OK
5) Xen XL-2-XML Parse new-disk ... OK
6) Xen XL-2-XML Format new-disk ... OK
7) Xen XL-2-XML Format disk-positional-parms-full ... OK
8) Xen XL-2-XML Format disk-positional-parms-partial ... Program received signal
SIGSEGV, Segmentation fault.
(gdb) where
xlcfg=0x66d2b0
"/home/wtenhave/WORK/libvirt/XOSS/BUGS/libvirt/tests/xlconfigdata/test-disk-positional-parms-partial.cfg",
xml=0x66d320
"/home/wtenhave/WORK/libvirt/XOSS/BUGS/libvirt/tests/xlconfigdata/test-disk-positional-parms-partial.xml",
replaceVars=false) at xlconfigtest.c:152
body=0x40f32d <testCompareHelper>, data=0x7fffffffd990) at testutils.c:180
(gdb) frame 1
319 if (STRPREFIX(srcstr, "rbd:")) {
(gdb) print srcstr
$1 = 0x0
Signed-off-by: Wim ten Have <wim.ten.have(a)oracle.com>
---
src/xenconfig/xen_xl.c | 3 ++-
tests/xlconfigdata/test-disk-positional-parms-partial.cfg | 2 +-
tests/xlconfigdata/test-disk-positional-parms-partial.xml | 6 ++++++
3 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
index 4f24d45..958956a 100644
--- a/src/xenconfig/xen_xl.c
+++ b/src/xenconfig/xen_xl.c
@@ -316,7 +316,8 @@ xenParseXLDiskSrc(virDomainDiskDefPtr disk, char *srcstr)
char *tmpstr = NULL;
int ret = -1;
- if (STRPREFIX(srcstr, "rbd:")) {
+ if (srcstr &&
+ STRPREFIX(srcstr, "rbd:")) {
if (!(tmpstr = virStringReplace(srcstr, "\\\\", "\\")))
goto cleanup;
diff --git a/tests/xlconfigdata/test-disk-positional-parms-partial.cfg
b/tests/xlconfigdata/test-disk-positional-parms-partial.cfg
index fd16db0..940304e 100644
--- a/tests/xlconfigdata/test-disk-positional-parms-partial.cfg
+++ b/tests/xlconfigdata/test-disk-positional-parms-partial.cfg
@@ -22,4 +22,4 @@ parallel = "none"
serial = "none"
builder = "hvm"
boot = "d"
-disk = [ "/dev/HostVG/XenGuest2,,hda,,backendtype=phy",
"/var/lib/libvirt/images/XenGuest2-home,,hdb,,",
"/root/boot.iso,,hdc,,devtype=cdrom" ]
+disk = [ "/dev/HostVG/XenGuest2,,hda,,backendtype=phy",
"/var/lib/libvirt/images/XenGuest2-home,,hdb,,",
"/root/boot.iso,,hdc,,devtype=cdrom" ,
"format=raw,vdev=hdd,access=ro,devtype=cdrom" ]
diff --git a/tests/xlconfigdata/test-disk-positional-parms-partial.xml
b/tests/xlconfigdata/test-disk-positional-parms-partial.xml
index e86a5be..52b21dc 100644
--- a/tests/xlconfigdata/test-disk-positional-parms-partial.xml
+++ b/tests/xlconfigdata/test-disk-positional-parms-partial.xml
@@ -39,6 +39,12 @@
<readonly/>
<address type='drive' controller='0' bus='1'
target='0' unit='0'/>
</disk>
+ <disk type='file' device='cdrom'>
+ <driver name='qemu' type='raw'/>
+ <target dev='hdd' bus='ide'/>
+ <readonly/>
+ <address type='drive' controller='0' bus='1'
target='0' unit='1'/>
+ </disk>
<controller type='ide' index='0'/>
<interface type='bridge'>
<mac address='00:16:3e:66:92:9c'/>
--
2.9.4
9:12 a.m.
On 05/19/2017 06:38 AM, Wim Ten Have wrote:
From: Wim ten Have <wim.ten.have(a)oracle.com>
Working larger code changes whilst testing functionality and domxml
conversion methodology for xen-xl (xenconfig) a cumbersome caveat surfaced
that potentially can take libvirtd out with a SEGV when parsing complex
disk xl.cfg directives.
This patch also includes tests/xlconfigdata adjustments to illustrate
specific disk xl.cfg directive and that way updating test 2-ways.
Running the tests with defensive code fix all will run without trouble,
running it without the code fix testing will trap with below listed
debug transcript.
<wtenhave@nina:21-ba$h> VIR_TEST_DEBUG=1 ./run gdb ./tests/xlconfigtest
TEST: xlconfigtest
1) Xen XL-2-XML Parse fullvirt-ovmf ... OK
2) Xen XL-2-XML Format fullvirt-ovmf ... OK
3) Xen XL-2-XML Parse paravirt-maxvcpus ... OK
4) Xen XL-2-XML Format paravirt-maxvcpus ... OK
5) Xen XL-2-XML Parse new-disk ... OK
6) Xen XL-2-XML Format new-disk ... OK
7) Xen XL-2-XML Format disk-positional-parms-full ... OK
8) Xen XL-2-XML Format disk-positional-parms-partial ... Program received signal
SIGSEGV, Segmentation fault.
(gdb) where
xlcfg=0x66d2b0
"/home/wtenhave/WORK/libvirt/XOSS/BUGS/libvirt/tests/xlconfigdata/test-disk-positional-parms-partial.cfg",
xml=0x66d320
"/home/wtenhave/WORK/libvirt/XOSS/BUGS/libvirt/tests/xlconfigdata/test-disk-positional-parms-partial.xml",
replaceVars=false) at xlconfigtest.c:152
body=0x40f32d <testCompareHelper>, data=0x7fffffffd990) at testutils.c:180
(gdb) frame 1
319 if (STRPREFIX(srcstr, "rbd:")) {
(gdb) print srcstr
$1 = 0x0
I'm always hesitant to critique verbose commit messages, but in this case I
think the real fix gets lost in the verbosity :-). IMO something along the lines
of the following would suffice:
xenconfig: fix handling of NULL disk source
It is possible to crash libvirtd when converting xl native config to domXML when
the xl config contains an empty disk source, e.g. an empty CDROM. Fix by
checking that the disk source is non-NULL before parsing it.
Signed-off-by: Wim ten Have <wim.ten.have(a)oracle.com>
---
src/xenconfig/xen_xl.c | 3 ++-
tests/xlconfigdata/test-disk-positional-parms-partial.cfg | 2 +-
tests/xlconfigdata/test-disk-positional-parms-partial.xml | 6 ++++++
3 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
index 4f24d45..958956a 100644
--- a/src/xenconfig/xen_xl.c
+++ b/src/xenconfig/xen_xl.c
@@ -316,7 +316,8 @@ xenParseXLDiskSrc(virDomainDiskDefPtr disk, char *srcstr)
char *tmpstr = NULL;
int ret = -1;
- if (STRPREFIX(srcstr, "rbd:")) {
+ if (srcstr &&
+ STRPREFIX(srcstr, "rbd:")) {
How about checking for a NULL source on entry to this function? Something like
the below diff?
Looks good otherwise!
Regards,
Jim
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
index 4f24d457c..cac440cd4 100644
--- a/src/xenconfig/xen_xl.c
+++ b/src/xenconfig/xen_xl.c
@@ -316,6 +316,10 @@ xenParseXLDiskSrc(virDomainDiskDefPtr disk, char *srcstr)
char *tmpstr = NULL;
int ret = -1;
+ /* A NULL source is valid, e.g. an empty CDROM */
+ if (srcstr == NULL)
+ return 0;
+
if (STRPREFIX(srcstr, "rbd:")) {
if (!(tmpstr = virStringReplace(srcstr, "\\\\", "\\")))
goto cleanup;
9:38 a.m.
On Fri, 19 May 2017 08:12:08 -0600
Jim Fehlig <jfehlig(a)suse.com> wrote:
On 05/19/2017 06:38 AM, Wim Ten Have wrote:
> From: Wim ten Have <wim.ten.have(a)oracle.com>
>
> Working larger code changes whilst testing functionality and domxml
> conversion methodology for xen-xl (xenconfig) a cumbersome caveat surfaced
> that potentially can take libvirtd out with a SEGV when parsing complex
> disk xl.cfg directives.
>
> This patch also includes tests/xlconfigdata adjustments to illustrate
> specific disk xl.cfg directive and that way updating test 2-ways.
>
> Running the tests with defensive code fix all will run without trouble,
> running it without the code fix testing will trap with below listed
> debug transcript.
>
> <wtenhave@nina:21-ba$h> VIR_TEST_DEBUG=1 ./run gdb ./tests/xlconfigtest
> TEST: xlconfigtest
> 1) Xen XL-2-XML Parse fullvirt-ovmf ... OK
> 2) Xen XL-2-XML Format fullvirt-ovmf ... OK
> 3) Xen XL-2-XML Parse paravirt-maxvcpus ... OK
> 4) Xen XL-2-XML Format paravirt-maxvcpus ... OK
> 5) Xen XL-2-XML Parse new-disk ... OK
> 6) Xen XL-2-XML Format new-disk ... OK
> 7) Xen XL-2-XML Format disk-positional-parms-full ... OK
> 8) Xen XL-2-XML Format disk-positional-parms-partial ... Program received signal
SIGSEGV, Segmentation fault.
>
> (gdb) where
> xlcfg=0x66d2b0
"/home/wtenhave/WORK/libvirt/XOSS/BUGS/libvirt/tests/xlconfigdata/test-disk-positional-parms-partial.cfg",
> xml=0x66d320
"/home/wtenhave/WORK/libvirt/XOSS/BUGS/libvirt/tests/xlconfigdata/test-disk-positional-parms-partial.xml",
replaceVars=false) at xlconfigtest.c:152
> body=0x40f32d <testCompareHelper>, data=0x7fffffffd990) at
testutils.c:180
>
> (gdb) frame 1
> 319 if (STRPREFIX(srcstr, "rbd:")) {
>
> (gdb) print srcstr
> $1 = 0x0
I'm always hesitant to critique verbose commit messages, but in this case I
think the real fix gets lost in the verbosity :-).
critique taken O:-)
IMO something along the lines of the following would suffice:
Exactly what is coming up.
xenconfig: fix handling of NULL disk source
It is possible to crash libvirtd when converting xl native config to domXML when
the xl config contains an empty disk source, e.g. an empty CDROM. Fix by
checking that the disk source is non-NULL before parsing it.
>
> Signed-off-by: Wim ten Have <wim.ten.have(a)oracle.com>
> ---
> src/xenconfig/xen_xl.c | 3 ++-
> tests/xlconfigdata/test-disk-positional-parms-partial.cfg | 2 +-
> tests/xlconfigdata/test-disk-positional-parms-partial.xml | 6 ++++++
> 3 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
> index 4f24d45..958956a 100644
> --- a/src/xenconfig/xen_xl.c
> +++ b/src/xenconfig/xen_xl.c
> @@ -316,7 +316,8 @@ xenParseXLDiskSrc(virDomainDiskDefPtr disk, char *srcstr)
> char *tmpstr = NULL;
> int ret = -1;
>
> - if (STRPREFIX(srcstr, "rbd:")) {
> + if (srcstr &&
> + STRPREFIX(srcstr, "rbd:")) {
How about checking for a NULL source on entry to this function? Something like
the below diff?
Looks good otherwise!
Regards,
Jim
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
index 4f24d457c..cac440cd4 100644
--- a/src/xenconfig/xen_xl.c
+++ b/src/xenconfig/xen_xl.c
@@ -316,6 +316,10 @@ xenParseXLDiskSrc(virDomainDiskDefPtr disk, char *srcstr)
char *tmpstr = NULL;
int ret = -1;
+ /* A NULL source is valid, e.g. an empty CDROM */
+ if (srcstr == NULL)
+ return 0;
+
if (STRPREFIX(srcstr, "rbd:")) {
if (!(tmpstr = virStringReplace(srcstr, "\\\\", "\\")))
goto cleanup;
2744
days inactive
2744
days old
3 comments
3 participants
participants (3)
-
Jim Fehlig -
Wim Ten Have -
Wim ten Have