2013/9/20 Daniel P. Berrange <berrange@xxxxxxxxxx>:
> On Thu, Sep 19, 2013 at 11:26:08PM +0900, Yuto KAWAMURA(kawamuray) wrote:
>> diff --git a/tools/wireshark/src/moduleinfo.h b/tools/wireshark/src/moduleinfo.h
>> new file mode 100644
>> index 0000000..9ab642c
>> --- /dev/null
>> +++ b/tools/wireshark/src/moduleinfo.h
>> @@ -0,0 +1,37 @@
>> +/* moduleinfo.h --- Define constants about wireshark plugin module
> ...
>> +
>> +/* Included *after* config.h, in order to re-define these macros */
>> +
>> +#ifdef PACKAGE
>> +# undef PACKAGE
>> +#endif
>> +
>> +/* Name of package */
>> +#define PACKAGE "libvirt"
>
> Huh ? "PACKAGE" will already be defined to 'libvirt' so why are
> you redefining it.
>
>> +
>> +
>> +#ifdef VERSION
>> +# undef VERSION
>> +#endif
>> +
>> +/* Version number of package */
>> +#define VERSION "0.0.1"
>
> This means the wireshark plugin will have a fixed version, even
> when libvirt protocol changes in new releases. This seems bogus.
> Again I think we should just use the existing defined "VERSION".
>
> I think this whole file can just go away completely
>
Right. I'll remove whole moduleinfo.h.
>> diff --git a/tools/wireshark/src/packet-libvirt.c b/tools/wireshark/src/packet-libvirt.c
>> new file mode 100644
>> index 0000000..cd3e6ce
>> --- /dev/null
>> +++ b/tools/wireshark/src/packet-libvirt.c
>> +static gboolean
>> +dissect_xdr_bytes(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int hf,
>> + guint32 maxlen)
>> +{
>> + goffset start;
>> + guint8 *val = NULL;
>> + guint32 length;
>> +
>> + start = xdr_getpos(xdrs);
>> + if (xdr_bytes(xdrs, (char **)&val, &length, maxlen)) {
>> + proto_tree_add_bytes_format_value(tree, hf, tvb, start, xdr_getpos(xdrs) - start,
>> + NULL, "%s", format_xdr_bytes(val, length));
>> + /* Seems I can't call xdr_free() for this case.
>> + It will raises SEGV by referencing out of bounds argument stack */
>> + xdrs->x_op = XDR_FREE;
>> + xdr_bytes(xdrs, (char **)&val, &length, maxlen);
>> + xdrs->x_op = XDR_DECODE;
>
> Is accessing the internals of the 'XDR' struct really portable ? I think
> it would be desirable to solve the xdr_free problem rather than accessing
> struct internals
>
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list