[libvirt] [PATCH] libxl: Fix possible invalid read

[Daniel P. Berrange]

On Thu, Oct 24, 2013 at 10:50:02AM +0100, Martin Kletzander wrote:
> According to the following valgrind output, there seems to be a
> invalid limit for the iterator (captured on Fedora 19):
> 
> ==3945== Invalid read of size 1
> ==3945==    at 0x1E1FA410: libxlVmStart (libxl_driver.c:475)
> ==3945==    by 0x1E1FAD9A: libxlDomainCreateWithFlags (libxl_driver.c:2633)
> ==3945==    by 0x5187D46: virDomainCreate (libvirt.c:9439)
> ==3945==    by 0x13BAA6: remoteDispatchDomainCreateHelper (remote_dispatch.h:2910)
> ==3945==    by 0x51DE5B9: virNetServerProgramDispatch (virnetserverprogram.c:435)
> ==3945==    by 0x51D93E7: virNetServerHandleJob (virnetserver.c:165)
> ==3945==    by 0x50F5BF4: virThreadPoolWorker (virthreadpool.c:144)
> ==3945==    by 0x50F5670: virThreadHelper (virthreadpthread.c:161)
> ==3945==    by 0x8046C52: start_thread (pthread_create.c:308)
> ==3945==    by 0x8758E1C: clone (clone.S:113)
> ==3945==  Address 0x23424d81 is 0 bytes after a block of size 1 alloc'd
> ==3945==    at 0x4A08121: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==3945==    by 0x50B1F8C: virAllocN (viralloc.c:189)
> ==3945==    by 0x1E1FA3CA: libxlVmStart (libxl_driver.c:468)
> ==3945==    by 0x1E1FAD9A: libxlDomainCreateWithFlags (libxl_driver.c:2633)
> ==3945==    by 0x5187D46: virDomainCreate (libvirt.c:9439)
> ==3945==    by 0x13BAA6: remoteDispatchDomainCreateHelper (remote_dispatch.h:2910)
> ==3945==    by 0x51DE5B9: virNetServerProgramDispatch (virnetserverprogram.c:435)
> ==3945==    by 0x51D93E7: virNetServerHandleJob (virnetserver.c:165)
> ==3945==    by 0x50F5BF4: virThreadPoolWorker (virthreadpool.c:144)
> ==3945==    by 0x50F5670: virThreadHelper (virthreadpthread.c:161)
> ==3945==    by 0x8046C52: start_thread (pthread_create.c:308)
> ==3945==    by 0x8758E1C: clone (clone.S:113)
> ==3945==
> 
> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1013045
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index e2a6d44..4928695 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -454,6 +454,7 @@ libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr driver, virDomainObjPtr vm)
>      size_t cpumaplen;
>      int vcpu;
>      size_t i;
> +    size_t limit;
>      int ret = -1;
> 
>      if (libxlDoNodeGetInfo(driver, &nodeinfo) < 0)
> @@ -470,7 +471,8 @@ libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr driver, virDomainObjPtr vm)
> 
>          cpumask = (uint8_t*) def->cputune.vcpupin[vcpu]->cpumask;
> 
> -        for (i = 0; i < VIR_DOMAIN_CPUMASK_LEN; ++i) {
> +        limit = MIN(VIR_DOMAIN_CPUMASK_LEN, cpumaplen);
> +        for (i = 0; i < limit; ++i) {
>              if (cpumask[i])
>                  VIR_USE_CPU(cpumap, i);
>          }

Hmm, what is strange about that bug is that the crash is in free() wrt
memory corruption, but valgrind only reports bogus reads. Such a crash
in free() usually implies a bogus write. The VIR_USE_CPU macro does
do such a write though, so the patch looks sane - valgrind must have
just not seen it

ACK

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|