Re: [libvirt] [PATCH v2 1/2] Introduce Libvirt Wireshark dissector

2013/9/20 Daniel P. Berrange <berrange(a)redhat.com&gt;: Right. I'll remove whole moduleinfo.h. I'll change this to use free(), but let me explain this problem detail. xdr_bytes may raises SEGV when it called from xdr_free. This is caused by xdr_free is accessing it's third argument 'sizep' even if it was called from xdr_free(in other word, when xdrs->x_op is XDR_FREE). This problem can't be reproduced in 64bit architecture due to 64bit system's register usage (I'll explain about this later). Following is a small enough code to reproduce this issue: #include <stdio.h> #include <stdlib.h> #include <rpc/xdr.h> /* Contents of this buffer is not important to reproduce the issue */ static char xdr_buffer[] = { 0x00, 0x00, 0x00, 0x02, /* length is 2byte */ 'A', '\0', 0, 0 /* 2 byte data and 2 byte padding bytes */ }; /* Same as the prototype of xdr_bytes() */ bool_t my_xdr_bytes(XDR *xdrs, char **cpp, u_int *sizep) { return TRUE; } /* Same as the prototype of xdr_free() */ void my_xdr_free(xdrproc_t proc, char *objp) { XDR x; (*proc)(&x, objp, NULL/* This NULL stored at same pos of 'sizep' in xdr_bytes() */); } int main(void) { XDR xdrs; char *opaque = NULL; unsigned int size; xdrmem_create(&xdrs, xdr_buffer, sizeof(xdr_buffer), XDR_DECODE); if (!xdr_bytes(&xdrs, &opaque, &size, ~0)) { fprintf(stderr, "xdr_bytes() returns FALSE\n"); exit(1); } /* Reproduce same stack-upping as call of xdr_free(xdr_bytes, &opaque). This is needed to stack-up 0x0(invalid address) on position of 'sizsp' which is third argument of xdr_bytes(). */ my_xdr_free((xdrproc_t)my_xdr_bytes, (char *)&opaque); /* *** SEGV!! *** */ xdr_free((xdrproc_t)xdr_bytes, (char *)&opaque); /* ************** */ xdr_destroy(&xdrs); return 0; } There are useless calls of my_xdr_free and my_xdr_bytes. These calls are used to reproduce same stack-upping of that when we call xdr_free((xdrproc_t)xdr_bytes, (char *)&allocated_mem). To reproduce this issue, we need to store some invalid address in callstack where it will referenced from xdr_bytes via it's third argument 'sizep'. These useless call will store NULL into the position on callstack which will be correspond to the position of 'sizep' in xdr_bytes. In rpc/xdr.h, xdrproc_t was typedefed as following: typedef bool_t (*xdrproc_t) (XDR *, void *,...); xdr_free will call passed xdrproc as function which takes two args and return bool_t. So called function from xdr_free should not access after second argument. But xdr_bytes is accessing it's third argument 'sizep' whatever it was called from xdr_free. This is totally wrong because we can't tell what is stored at position of after second argument in call stack. This problem can only be reproduced in 32bit environment due to register usage on function call. Following explanation is regarding to my environment, so it may not be reproduced in your environment. Compiler and library info: - glibc : 2.15 installed from Portage - gdb : GNU gdb (Gentoo 7.5.1 p2) 7.5.1 * x86 32bit - uname -rmpio : 3.10.7-gentoo i686 Intel(R) Core(TM) i7 CPU 860 @ 2.80GHz GenuineIntel GNU/Linux - gcc : gcc version 4.6.3 (Gentoo 4.6.3 p1.9, pie-0.5.2) * x86_64 64bit - uname -rmpio : 3.8.13-gentoo x86_64 Intel(R) Xeon(R) CPU E5310 @ 1.60GHz GenuineIntel GNU/Linux - gcc : gcc version 4.6.3 (Gentoo 4.6.3 p1.13, pie-0.5.2) On x86(32bit), xdr_free and xdr_bytes were compiled as followings: void xdr_free(xdrproc_t proc, char *objp): #0 __GI_xdr_free (proc=0x8048430 <xdr_bytes@plt>, objp=0xbffff4e4 "\b\260\004\b\001") at xdr.c:66 Dump of assembler code for function __GI_xdr_free: => 0xb7f35fd0 <+0>: sub $0x3c,%esp 0xb7f35fd3 <+3>: mov 0x44(%esp),%eax 0xb7f35fd7 <+7>: movl $0x2,0x18(%esp) 0xb7f35fdf <+15>: mov %eax,0x4(%esp) 0xb7f35fe3 <+19>: lea 0x18(%esp),%eax 0xb7f35fe7 <+23>: mov %eax,(%esp) 0xb7f35fea <+26>: call *0x40(%esp) 0xb7f35fee <+30>: add $0x3c,%esp 0xb7f35ff1 <+33>: ret bool_t xdr_bytes(XDR *xdrs, char **cpp, u_int *sizep, u_int maxsize): #0 __GI_xdr_bytes (xdrs=0xbffff4a8, cpp=0xbffff4e4, sizep=0x0, maxsize=3085225635) at xdr.c:608 Dump of assembler code for function __GI_xdr_bytes: 0xb7f365b0 <+0>: sub $0x3c,%esp 0xb7f365b3 <+3>: mov %ebp,0x38(%esp) 0xb7f365b7 <+7>: mov 0x4c(%esp),%edx 0xb7f365bb <+11>: mov 0x44(%esp),%ebp 0xb7f365bf <+15>: mov %ebx,0x2c(%esp) 0xb7f365c3 <+19>: call 0xb7f417e3 <__i686.get_pc_thunk.bx> 0xb7f365c8 <+24>: add $0x82a38,%ebx 0xb7f365ce <+30>: mov %esi,0x30(%esp) 0xb7f365d2 <+34>: mov 0x40(%esp),%esi 0xb7f365d6 <+38>: mov %edi,0x34(%esp) 0xb7f365da <+42>: mov 0x48(%esp),%edi 0xb7f365de <+46>: mov %edx,0x18(%esp) 0xb7f365e2 <+50>: mov 0x0(%ebp),%edx 0xb7f365e5 <+53>: mov %esi,(%esp) 0xb7f365e8 <+56>: mov %edi,0x4(%esp) 0xb7f365ec <+60>: mov %edx,0x1c(%esp) 0xb7f365f0 <+64>: call 0xb7f36070 <__GI_xdr_u_long> 0xb7f365f5 <+69>: test %eax,%eax 0xb7f365f7 <+71>: je 0xb7f36660 <__GI_xdr_bytes+176> => 0xb7f365f9 <+73>: mov (%edi),%edi On x86_64(64bit), xdr_free and xdr_bytes were compiled as followings: void xdr_free(xdrproc_t proc, char *objp): #0 __GI_xdr_free (proc=0x400590 <xdr_bytes@plt>, objp=0x7fffffffe248 "\020 `") at xdr.c:66 Dump of assembler code for function __GI_xdr_free: => 0x00007ffff7b4b880 <+0>: sub $0x38,%rsp 0x00007ffff7b4b884 <+4>: mov %rdi,%rdx 0x00007ffff7b4b887 <+7>: xor %eax,%eax 0x00007ffff7b4b889 <+9>: movl $0x2,(%rsp) 0x00007ffff7b4b890 <+16>: mov %rsp,%rdi 0x00007ffff7b4b893 <+19>: callq *%rdx 0x00007ffff7b4b895 <+21>: add $0x38,%rsp 0x00007ffff7b4b899 <+25>: retq bool_t xdr_bytes(XDR *xdrs, char **cpp, u_int *sizep, u_int maxsize): #0 __GI_xdr_bytes (xdrs=0x7fffffffe200, cpp=0x7fffffffe248, sizep=0x400590 <xdr_bytes@plt>, maxsize=4294959688) at xdr.c:608 Dump of assembler code for function __GI_xdr_bytes: 0x00007ffff7b4bf00 <+0>: mov %rbx,-0x28(%rsp) 0x00007ffff7b4bf05 <+5>: mov %rdi,%rbx 0x00007ffff7b4bf08 <+8>: mov %rbp,-0x20(%rsp) 0x00007ffff7b4bf0d <+13>: mov %rsi,%rbp 0x00007ffff7b4bf10 <+16>: mov %r12,-0x18(%rsp) 0x00007ffff7b4bf15 <+21>: mov %rdx,%r12 0x00007ffff7b4bf18 <+24>: mov %r14,-0x8(%rsp) 0x00007ffff7b4bf1d <+29>: mov %ecx,%r14d 0x00007ffff7b4bf20 <+32>: mov %r13,-0x10(%rsp) 0x00007ffff7b4bf25 <+37>: sub $0x38,%rsp 0x00007ffff7b4bf29 <+41>: mov (%rsi),%r13 0x00007ffff7b4bf2c <+44>: mov %rdx,%rsi 0x00007ffff7b4bf2f <+47>: callq 0x7ffff7b4b920 <xdr_u_int> 0x00007ffff7b4bf34 <+52>: test %eax,%eax 0x00007ffff7b4bf36 <+54>: je 0x7ffff7b4bfb0 <__GI_xdr_bytes+176> => 0x00007ffff7b4bf38 <+56>: mov (%r12),%edx So these differences are why my code will raises SEGV only on x86 environment. In x86_64(64bit), gcc does positively uses register(s) instead of callstack to pass arguments to called function. As you can see in above assembly, 'sizep' of xdr_bytes is referencing register %rdx as location of it's value should sotred. But xdr_free does not store value in %rdx as an argument for xdr_bytes. It does only uses %rdx as a temporary location for previous processing. xdr_bytes is referencing %rdx for it's third argument. As you can see at 0x00007ffff7b4b884, it does only uses %rdx as temporary register. So it still keeps value 0x400590(address of xdr_bytes) and it wouldn't be a problem even if xdr_bytes will try to dereference it(because it's not a invalid address!). This is why my code doesn't raises SEGV in x86_64 environment. Anyway, xdr_bytes is accessing 'sizep' even if it was called from xdr_free. It is totally wrong and should be fixed. Appendix - output of valgrind on my 32bit system $ valgrind ./xdr_free_segv_reproduce ==2778== Memcheck, a memory error detector ==2778== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al. ==2778== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info ==2778== Command: ./xdr_free_segv_reproduce ==2778== ==2778== Use of uninitialised value of size 4 ==2778== at 0x416D5F9: xdr_bytes (xdr.c:608) ==2778== ==2778== Invalid read of size 4 ==2778== at 0x416D5F9: xdr_bytes (xdr.c:608) ==2778== Address 0x0 is not stack'd, malloc'd or (recently) free'd ==2778== ==2778== ==2778== Process terminating with default action of signal 11 (SIGSEGV) ==2778== Access not within mapped region at address 0x0 ==2778== at 0x416D5F9: xdr_bytes (xdr.c:608) ==2778== If you believe this happened as a result of a stack ==2778== overflow in your program's main thread (unlikely but ==2778== possible), you can try to increase the size of the ==2778== main thread stack using the --main-stacksize= flag. ==2778== The main thread stack size used in this run was 8388608. ==2778== ==2778== HEAP SUMMARY: ==2778== in use at exit: 2 bytes in 1 blocks ==2778== total heap usage: 1 allocs, 0 frees, 2 bytes allocated ==2778== ==2778== LEAK SUMMARY: ==2778== definitely lost: 0 bytes in 0 blocks ==2778== indirectly lost: 0 bytes in 0 blocks ==2778== possibly lost: 0 bytes in 0 blocks ==2778== still reachable: 2 bytes in 1 blocks ==2778== suppressed: 0 bytes in 0 blocks ==2778== Rerun with --leak-check=full to see details of leaked memory ==2778== ==2778== For counts of detected and suppressed errors, rerun with: -v ==2778== Use --track-origins=yes to see where uninitialised values come from ==2778== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0) zsh: segmentation fault valgrind ./xdr_free_segv_reproduce