[libvirt] [PATCH] util: Fix crash of libvirtd when running numatune with invalid nodeset
This issue is introduced by commit 0fc8909, the virBitmapIsSet() needs caller to ensure 'b < bitmap->max_bit', but it's lost in the virBitmapParse() caller, this will cause crash of libvirtd, with the patch, libvirtd no crash and can get a expected error "Failed to parse nodeset". How to reproduce? # virsh numatune foo --nodeset 1000000000 Actual result: error: Unable to change numa parameters error: End of file while reading data: Input/output error error: One or more references were leaked after disconnect from the hypervisor error: Failed to reconnect to the hypervisor GDB backtrace: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7f80ee533700 (LWP 4156)] 0x00007f80fb1b8906 in virBitmapIsSet (str=<value optimized out>, terminator=0 '\000', bitmap=0x7f80ee532978, bitmapSize=<value optimized out>) at util/virbitmap.c:158 158 return !!(bitmap->map[VIR_BITMAP_UNIT_OFFSET(b)] & VIR_BITMAP_BIT(b)); (gdb) bt #0 0x00007f80fb1b8906 in virBitmapIsSet (str=<value optimized out>, terminator=0 '\000', bitmap=0x7f80ee532978, bitmapSize=<value optimized out>) at util/virbitmap.c:158 #1 virBitmapParse (str=<value optimized out>, terminator=0 '\000', bitmap=0x7f80ee532978, bitmapSize=<value optimized out>) at util/virbitmap.c:351 #2 0x00007f80e5e7ef6b in qemuDomainSetNumaParameters (dom=<value optimized out>, params=<value optimized out>, nparams=1, flags=2) at qemu/qemu_driver.c:8357 #3 0x00007f80fb2ad7c6 in virDomainSetNumaParameters (domain=0x7f80d4000930, params=0x7f80d4000b00, nparams=1, flags=0) at libvirt.c:4163 #4 0x00007f80fbcd17ec in remoteDispatchDomainSetNumaParameters (server=<value optimized out>, client=<value optimized out>, msg=<value optimized out>, rerr=0x7f80ee532ba0, args=<value optimized out>, ret=<value optimized out>) at remote_dispatch.h:7771 #5 remoteDispatchDomainSetNumaParametersHelper (server=<value optimized out>, client=<value optimized out>, msg=<value optimized out>, rerr=0x7f80ee532ba0, args=<value optimized out>, ret=<value optimized out>) at remote_dispatch.h:7741 #6 0x00007f80fb2f6518 in virNetServerProgramDispatchCall (prog=0x7f80fd33a990, server=0x7f80fd32f3e0, client=0x7f80fd33fe30, msg=0x7f80fd33e550) at rpc/virnetserverprogram.c:435 #7 virNetServerProgramDispatch (prog=0x7f80fd33a990, server=0x7f80fd32f3e0, client=0x7f80fd33fe30, msg=0x7f80fd33e550) at rpc/virnetserverprogram.c:305 #8 0x00007f80fb2f9dd6 in virNetServerProcessMsg (srv=<value optimized out>, client=0x7f80fd33fe30, prog=<value optimized out>, msg=0x7f80fd33e550) at rpc/virnetserver.c:165 #9 0x00007f80fb2fa9f3 in virNetServerHandleJob (jobOpaque=<value optimized out>, opaque=<value optimized out>) at rpc/virnetserver.c:186 #10 0x00007f80fb20210e in virThreadPoolWorker (opaque=<value optimized out>) at util/virthreadpool.c:144 #11 0x00007f80fb2016f6 in virThreadHelper (data=<value optimized out>) at util/virthreadpthread.c:161 #12 0x00007f80f8d857f1 in start_thread (arg=0x7f80ee533700) at pthread_create.c:301 #13 0x00007f80f86bb70d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:115 RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=997367 Signed-off-by: Alex Jia <ajia@redhat.com> --- The caller virBitmapGetBit() can make sure 'b < bitmap->max_bit', so don't need to worry about higher caller for the virBitmapGetBit(), but the virBitmapParse() is called by many XML parser function, not sure which one can crash libvirtd with read-only client then probably require a CVE, I haven't a good way to check them now and only manually check them one by one. src/util/virbitmap.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 7e1cd02..edbfb30 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -337,6 +337,9 @@ virBitmapParse(const char *str, if (start < 0) goto parse_error; + if ((*bitmap)->max_bit <= start) + goto parse_error; + cur = tmp; virSkipSpaces(&cur); -- 1.7.1
On 08/16/13 09:47, Alex Jia wrote:
This issue is introduced by commit 0fc8909, the virBitmapIsSet() needs caller to ensure 'b < bitmap->max_bit', but it's lost in the virBitmapParse() caller, this will cause crash of libvirtd, with the patch, libvirtd no crash and can get a expected error "Failed to parse nodeset".
How to reproduce?
# virsh numatune foo --nodeset 1000000000 Actual result: error: Unable to change numa parameters error: End of file while reading data: Input/output error error: One or more references were leaked after disconnect from the hypervisor error: Failed to reconnect to the hypervisor
GDB backtrace:
....
RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=997367
Signed-off-by: Alex Jia <ajia@redhat.com> --- The caller virBitmapGetBit() can make sure 'b < bitmap->max_bit', so don't need to worry about higher caller for the virBitmapGetBit(), but the virBitmapParse() is called by many XML parser function, not sure which one can crash libvirtd with read-only client then probably require a CVE, I haven't a good way to check them now and only manually check them one by one.
src/util/virbitmap.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 7e1cd02..edbfb30 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -337,6 +337,9 @@ virBitmapParse(const char *str, if (start < 0) goto parse_error;
+ if ((*bitmap)->max_bit <= start) + goto parse_error; +
This will check only the start value of a range. When you use a range that starts with a valid number "1-1000000000000" for example, then the loop that is setting bits from the range will triger the crash too. IMO the correct fix here is to get rid of the weird construction: if (!virBitmapIsSet(*bitmap, i)) { ignore_value(virBitmapSetBit(*bitmap, i)); ret++; } with if (virBitmapSetBit(*bitmap, i) < 0) goto error; and at the end count the enabled bits as ret = virBitmapCountBits(*bitmap); instead of the code that is present. I'll post an updated version of this patch containing the suggested changes. Peter
On 08/16/2013 01:47 AM, Alex Jia wrote:
This issue is introduced by commit 0fc8909, the virBitmapIsSet() needs caller to ensure 'b < bitmap->max_bit', but it's lost in the virBitmapParse() caller, this will cause crash of libvirtd, with the patch, libvirtd no crash and can get a expected error "Failed to parse nodeset".
--- The caller virBitmapGetBit() can make sure 'b < bitmap->max_bit', so don't need to worry about higher caller for the virBitmapGetBit(), but the virBitmapParse() is called by many XML parser function, not sure which one can crash libvirtd with read-only client then probably require a CVE, I haven't a good way to check them now and only manually check them one by one.
If you are worried that a bug might be a CVE, it is best to practice responsible disclosure, and NOT post the patch upstream, but instead post to libvirt-security@redhat.com. That way, the problem can be discussed without public disclosure, rather than calling attention to the fact and making it easier to design a 0-day exploit. But now that this is already publicly disclosed, we have to hurry up both the fix, and our analysis of whether it is exploitable. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/16/2013 07:42 PM, Eric Blake wrote:
This issue is introduced by commit 0fc8909, the virBitmapIsSet() needs caller to ensure 'b< bitmap->max_bit', but it's lost in the virBitmapParse() caller, this will cause crash of libvirtd, with the patch, libvirtd no crash and can get a expected error "Failed to parse nodeset".
--- The caller virBitmapGetBit() can make sure 'b< bitmap->max_bit', so don't need to worry about higher caller for the virBitmapGetBit(), but the virBitmapParse() is called by many XML parser function, not sure which one can crash libvirtd with read-only client then probably require a CVE, I haven't a good way to check them now and only manually check them one by one. If you are worried that a bug might be a CVE, it is best to practice responsible disclosure, and NOT post the patch upstream, but instead
On 08/16/2013 01:47 AM, Alex Jia wrote: post to libvirt-security@redhat.com. That way, the problem can be
Got it, if I think a bug might be a CVE then will post the patch to libvirt-security@redhat.com next time, thanks.
discussed without public disclosure, rather than calling attention to the fact and making it easier to design a 0-day exploit. But now that this is already publicly disclosed, we have to hurry up both the fix, and our analysis of whether it is exploitable.
participants (3)
-
Alex Jia -
Eric Blake -
Peter Krempa