[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 20.01.2014 15:04, Yuto KAWAMURA wrote:
> 2014/1/16 Michal Privoznik <mprivozn redhat com>:
>> On 15.01.2014 18:06, Yuto KAWAMURA(kawamuray) wrote:
>>> From: "Yuto KAWAMURA(kawamuray)" <kawamuray dadada gmail com>
>>>
>>> Introduce Wireshark dissector plugin which adds support to Wireshark
>>> for dissecting libvirt RPC protocol.
>>> Added following files to build Wireshark dissector from libvirt source
>>> tree.
>>> * tools/wireshark/*: Source tree of Wireshark dissector plugin.
>>>
>>> Added followings to configure.ac or Makefile.am.
>>> configure.ac
>>> * --with-wireshark-dissector: Enable support for building Wireshark
>>>   dissector.
>>> * --with-ws-plugindir: Specify wireshark plugin directory that dissector
>>>   will installed.
>>> * Added tools/wireshark/{Makefile,src/Makefile} to  AC_CONFIG_FILES.
>>> Makefile.am
>>> * Added tools/wireshark/ to SUBDIR.
>>> ---
>>>  .gitignore                              |    2 +
>>>  Makefile.am                             |    3 +-
>>>  cfg.mk                                  |    8 +-
>>>  configure.ac                            |   72 ++-
>>>  tools/wireshark/Makefile.am             |   22 +
>>>  tools/wireshark/README.md               |   31 +
>>>  tools/wireshark/src/Makefile.am         |   42 ++
>>>  tools/wireshark/src/packet-libvirt.c    |  512 ++++++++++++++++
>>>  tools/wireshark/src/packet-libvirt.h    |  128 ++++
>>>  tools/wireshark/util/genxdrstub.pl      | 1011 +++++++++++++++++++++++++++++++
>>>  tools/wireshark/util/make-dissector-reg |  198 ++++++
>>>  11 files changed, 2023 insertions(+), 6 deletions(-)
>>>  create mode 100644 tools/wireshark/Makefile.am
>>>  create mode 100644 tools/wireshark/README.md
>>>  create mode 100644 tools/wireshark/src/Makefile.am
>>>  create mode 100644 tools/wireshark/src/packet-libvirt.c
>>>  create mode 100644 tools/wireshark/src/packet-libvirt.h
>>>  create mode 100755 tools/wireshark/util/genxdrstub.pl
>>>  create mode 100755 tools/wireshark/util/make-dissector-reg
>>>
>>
>>> diff --git a/tools/wireshark/src/packet-libvirt.c b/tools/wireshark/src/packet-libvirt.c
>>> new file mode 100644
>>> index 0000000..2d0350c
>>> --- /dev/null
>>> +++ b/tools/wireshark/src/packet-libvirt.c
>>
>>> +static void
>>> +dissect_libvirt_message(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
>>> +{
>>> +    goffset offset;
>>> +    guint32 prog, proc, type, serial, status;
>>> +    const value_string *vs;
>>> +
>>> +    col_set_str(pinfo->cinfo, COL_PROTOCOL, "Libvirt");
>>> +    col_clear(pinfo->cinfo, COL_INFO);
>>> +
>>> +    offset = 4; /* End of length field */
>>> +    prog   = tvb_get_ntohl(tvb, offset); offset += 4;
>>> +    offset += 4; /* Ignore version header field */
>>> +    proc   = tvb_get_ntohl(tvb, offset); offset += 4;
>>> +    type   = tvb_get_ntohl(tvb, offset); offset += 4;
>>> +    serial = tvb_get_ntohl(tvb, offset); offset += 4;
>>> +    status = tvb_get_ntohl(tvb, offset); offset += 4;
>>> +
>>> +    col_add_fstr(pinfo->cinfo, COL_INFO, "Prog=%s",
>>> +                 val_to_str(prog, program_strings, "%x"));
>>> +
>>> +    vs = get_program_data(prog, VIR_PROGRAM_PROCSTRINGS);
>>> +    if (vs == NULL) {
>>> +        col_append_fstr(pinfo->cinfo, COL_INFO, " Proc=%u", proc);
>>> +    } else {
>>> +        col_append_fstr(pinfo->cinfo, COL_INFO, " Proc=%s", val_to_str(proc, vs, "%d"));
>>> +    }
>>> +
>>> +    col_append_fstr(pinfo->cinfo, COL_INFO, " Type=%s Serial=%u Status=%s",
>>> +                    val_to_str(type, type_strings, "%d"), serial,
>>> +                    val_to_str(status, status_strings, "%d"));
>>> +
>>> +    if (tree) {
>>> +        gint hf_proc;
>>> +        proto_item *ti;
>>> +        proto_tree *libvirt_tree;
>>> +
>>> +        ti = proto_tree_add_item(tree, proto_libvirt, tvb, 0, tvb_length(tvb), ENC_NA);
>>> +        libvirt_tree = proto_item_add_subtree(ti, ett_libvirt);
>>> +
>>> +        offset = 0;
>>> +        proto_tree_add_item(libvirt_tree, hf_libvirt_length,  tvb, offset, 4, ENC_NA); offset += 4;
>>> +        proto_tree_add_item(libvirt_tree, hf_libvirt_program, tvb, offset, 4, ENC_NA); offset += 4;
>>> +        proto_tree_add_item(libvirt_tree, hf_libvirt_version, tvb, offset, 4, ENC_NA); offset += 4;
>>> +
>>> +        hf_proc = *(int *)get_program_data(prog, VIR_PROGRAM_PROCHFVAR);
>>
>> There's a possible NULL dereference here. get_program might return NULL in case @prog is not one of REMOTE, QEMU, LXC, KEEPALIVE. This can possibly happen and it did for me indeed:
>>
>> Program terminated with signal 11, Segmentation fault.
>> #0  0x00007fac1cc60a86 in dissect_libvirt_message (tvb=0x387acc0, pinfo=0x7fffb9a03100, tree=0x3883090) at packet-libvirt.c:396
>> 396             hf_proc = *(int *)get_program_data(prog, VIR_PROGRAM_PROCHFVAR);
>> (gdb) bt
>> #0  0x00007fac1cc60a86 in dissect_libvirt_message (tvb=0x387acc0, pinfo=0x7fffb9a03100, tree=0x3883090) at packet-libvirt.c:396
>> #1  0x00000034469198fe in tcp_dissect_pdus () from /usr/lib64/libwireshark.so.3
>> #2  0x00007fac1cc60c0c in dissect_libvirt (tvb=0x3879aa0, pinfo=0x7fffb9a03100, tree=0x3883090) at packet-libvirt.c:424
>> #3  0x00000034462dff54 in ?? () from /usr/lib64/libwireshark.so.3
>> #4  0x00000034462e0608 in ?? () from /usr/lib64/libwireshark.so.3
>> #5  0x00000034462e0e0c in ?? () from /usr/lib64/libwireshark.so.3
>> #6  0x00000034462e0e67 in dissector_try_uint () from /usr/lib64/libwireshark.so.3
>> ...
>>
>> (gdb) p /x VIR_PROGRAM_PROCHFVAR
>> $1 = 0x0
>> (gdb) p /x prog
>> $2 = 0xd21c44f9
>>
>> This happened if I used sasl for authentication (auth_tcp="sasl" listen_tcp=1 listen_tls=0)
>>
>>> +        if (hf_proc == -1) {
>>> +            proto_tree_add_none_format(libvirt_tree, -1, tvb, offset, 4, "Unknown proc: %u", proc);
>>> +        } else {
>>> +            proto_tree_add_item(libvirt_tree, hf_proc, tvb, offset, 4, ENC_NA);
>>> +        }
>>> +        offset += 4;
>>
>> I've tried to fixed this by applying:
>> @@ -381,7 +381,7 @@ dissect_libvirt_message(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
>>                      val_to_str(status, status_strings, "%d"));
>>
>>      if (tree) {
>> -        gint hf_proc;
>> +        gint *hf_proc;
>>          proto_item *ti;
>>          proto_tree *libvirt_tree;
>>
>> @@ -393,11 +393,11 @@ dissect_libvirt_message(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
>>          proto_tree_add_item(libvirt_tree, hf_libvirt_program, tvb, offset, 4, ENC_NA); offset += 4;
>>          proto_tree_add_item(libvirt_tree, hf_libvirt_version, tvb, offset, 4, ENC_NA); offset += 4;
>>
>> -        hf_proc = *(int *)get_program_data(prog, VIR_PROGRAM_PROCHFVAR);
>> -        if (hf_proc == -1) {
>> +        hf_proc = (int *)get_program_data(prog, VIR_PROGRAM_PROCHFVAR);
>> +        if (!hf_proc || *hf_proc == -1) {
>>              proto_tree_add_none_format(libvirt_tree, -1, tvb, offset, 4, "Unknown proc: %u", proc);
>>          } else {
>> -            proto_tree_add_item(libvirt_tree, hf_proc, tvb, offset, 4, ENC_NA);
>> +            proto_tree_add_item(libvirt_tree, *hf_proc, tvb, offset, 4, ENC_NA);
>>          }
>>          offset += 4;
>>
>>
>> but it worked only partially as then I was seeing this error messages (but no segfault :) ):
>>
>> 12:29:09          Warn Dissector bug, protocol libvirt, in packet 32: proto.c:1854: failed assertion "(guint)hfindex < gpa_hfinfo.len" (Unregistered hf!)
>>
>> The error message kept repeating over and over again. The rest of the patch looks okay. Once you fix this (sending a follow-up patch is just fine) I'll push this. Thanks for taking care of the dissector!
>>
>> Michal
>>
> 
> Thanks for review Michal.
> Fix patch for this bug is following.
> Regoards.
> 
> diff --git a/tools/wireshark/src/packet-libvirt.c
> b/tools/wireshark/src/packet-libvirt.c
> index 2d0350c..d64ecce 100644
> --- a/tools/wireshark/src/packet-libvirt.c
> +++ b/tools/wireshark/src/packet-libvirt.c
> @@ -39,6 +39,7 @@ static int proto_libvirt = -1;
>  static int hf_libvirt_length = -1;
>  static int hf_libvirt_program = -1;
>  static int hf_libvirt_version = -1;
> +static int hf_libvirt_procedure = -1;
>  static int hf_libvirt_type = -1;
>  static int hf_libvirt_serial = -1;
>  static int hf_libvirt_status = -1;
> @@ -381,7 +382,7 @@ dissect_libvirt_message(tvbuff_t *tvb, packet_info
> *pinfo, proto_tree *tree)
>                      val_to_str(status, status_strings, "%d"));
> 
>      if (tree) {
> -        gint hf_proc;
> +        gint *hf_proc;
>          proto_item *ti;
>          proto_tree *libvirt_tree;
> 
> @@ -393,11 +394,12 @@ dissect_libvirt_message(tvbuff_t *tvb,
> packet_info *pinfo, proto_tree *tree)
>          proto_tree_add_item(libvirt_tree, hf_libvirt_program, tvb,
> offset, 4, ENC_NA); offset += 4;
>          proto_tree_add_item(libvirt_tree, hf_libvirt_version, tvb,
> offset, 4, ENC_NA); offset += 4;
> 
> -        hf_proc = *(int *)get_program_data(prog, VIR_PROGRAM_PROCHFVAR);
> -        if (hf_proc == -1) {
> -            proto_tree_add_none_format(libvirt_tree, -1, tvb, offset,
> 4, "Unknown proc: %u", proc);
> +        hf_proc = (int *)get_program_data(prog, VIR_PROGRAM_PROCHFVAR);
> +        if (hf_proc != NULL && *hf_proc != -1) {
> +            proto_tree_add_item(libvirt_tree, *hf_proc, tvb, offset,
> 4, ENC_NA);
>          } else {
> -            proto_tree_add_item(libvirt_tree, hf_proc, tvb, offset, 4, ENC_NA);
> +            /* No string representation, but still useful displaying
> proc number */
> +            proto_tree_add_item(libvirt_tree, hf_libvirt_procedure,
> tvb, offset, 4, ENC_NA);
>          }
>          offset += 4;
> 
> @@ -446,6 +448,12 @@ proto_register_libvirt(void)
>              NULL, 0x0,
>              NULL, HFILL}
>          },
> +        { &hf_libvirt_procedure,
> +          { "procedure", "libvirt.procedure",
> +            FT_INT32, BASE_DEC,
> +            NULL, 0x0,
> +            NULL, HFILL}
> +        },
>          { &hf_libvirt_type,
>            { "type", "libvirt.type",
>              FT_INT32, BASE_DEC,
> 
>

Yes, this fixes the problem. ACK. I'm squashing this in and pushing the
series. Thanks!

Michal


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]