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

2014/1/16 Michal Privoznik <mprivozn(a)redhat.com&gt;: > On 15.01.2014 18:06, Yuto KAWAMURA(kawamuray) wrote: >> From: "Yuto KAWAMURA(kawamuray)" <kawamuray.dadada(a)gmail.com&gt; >> >> 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,