[Denis 'GNUtoo' Carikli <GNUtoo@xxxxxxxxxxxxxxxxxx>]

On Tue, 22 Feb 2022 18:03:30 +0200
ss ws <sswsdev@xxxxxxxxx> wrote:

> Hi Denis,
Hi,

> I found myself writing several "multi payloads" dissectors like the
> one you describe and I can say the best thing you can do is write a
> compact description of each packet into the info column and separate
> the payloads:
> * Visually using commas
> * And logicaly using something called a 'fence'-- look for other
> dissectors calling 'col_set_fence'
Thanks a lot, I've now implemented that and it works fine. I'll
probably write some script to reformat the tshark output to be more
diff/meld friendly.

> About the second issue:
> I'll let someone else answer but I remember vaugly that filters aren't
> exclusive so you might be able to just register both with the same
> filter string. An alternative I do remember is the trick used for
> "ip.addr" vs "ip.src" and "ip.dst" fields.
> Both "Source Address" and "Destination Address" have their own fields
> with unique filters but by also the dissector is adding a hidden
> field for each of them (the one with the ip.addr filter) so you can
> filter for both of those fields using the unified filter.
> I'd recommend reading that part in the IP dissector for inspiration.
The issue with "ip.addr" is that data is "generated" from custom code
but the interpretation of that data isn't.

In my case the field is fixed but its interpretation changes depending
on if it's an incoming or outgoing usb packet (P2P_DIR_RECV
or P2P_DIR_SENT).

After some trial and errors I've managed to get something that works
but there is probably a better way to do it.

Basically I register it like that without interpreting the field:
> void proto_register_samsung_ipc(void)
> {
> 	static hf_register_info hf_fmt[] = {
>       [...]
> 		{ &hf_samsung_ipc_fmt_type,
> 		  { "samsung-ipc FMT type",
> 		    "samsung-ipc.fmt.type",
> 		    FT_UINT8,
> 		    BASE_HEX,
> 		    NULL,
> 		    0x0,
> 		    NULL,
> 		    HFILL },
> 		},
>               [...]
> 	};
>       [...]
> 	proto_register_field_array(proto_samsung_ipc,
> 				   hf_fmt, array_length(hf_fmt));

And before I just didn't decode the values:
> item = proto_tree_add_item(fmt_tree,
> 			     hf_samsung_ipc_fmt_type, tvb, 9,
> 			     1, ENC_NA);

And now I decode it by setting the field string:
> fmt_type = tvb_get_guint8(tvb, 9);
> if (usb_conv_info->direction == P2P_DIR_RECV)
> 	fmt_type_str = try_val_to_str(
>				fmt_type,
> 				hf_samsung_ipc_fmt_requests_types);
> else if (usb_conv_info->direction == P2P_DIR_SENT)
> 	fmt_type_str = try_val_to_str(
> 				fmt_type,
> 				hf_samsung_ipc_fmt_responses_types);
> item = proto_tree_add_item(
> 			fmt_tree,
> 			hf_samsung_ipc_fmt_type, tvb, 9, 1,
> 			ENC_NA);
> if (fmt_type_str)
> 	proto_item_set_text(item,
> 			    "samsung-ipc FMT type: %s (0x%x)",
> 			    fmt_type_str, fmt_type);

So now I'll cleanup the code. I'll also try to get some packet traces
of the Nokia ISI protocol to see how things are displayed to do
something similar with the samsung-ipc dissector.

I've some more questions on what is acceptable or not for upstreaming
(a samsung-ipc) dissector(s):
- The samsung-ipc protocol is a protocol to communicate with some modems
  that run firmwares made by/for Samsung. So like AT commands or other
  modem protocols we can send commands to it (like call this number) and
  receive information (like we have a call from this number).

  Is it OK if only the basics are implemented (like the commands,
  sequence numbers, etc), but not necessarily more advanced information?

  In the example above it would report the command CALL_OUTGOING 
  but not parse the command data (that includes the phone number and
  other parameters).

- The free software implementation(s) of that protocol consist in two
  parts:
  - kernel drivers that don't do much encoding/decoding beside
    adding/removing HDLC frame delimiters and separating channels and
    direction
  - an userspace library which implements the protocol. It was written
    by (originally not by me) by looking at protocol traces.
  I've used the same terminology names than the struct fields in
  libsamsung-ipc, to stay consistent, even if the field names could be
  improved. Is that OK?

- After the HDLC frame delimiters there is 3 bytes before the
  samsung-ipc packet length, and I've not yet found where in the
  drivers or in libsamsung-ipc there is that offset or where it could
  come from.

- The channels are encoded in the USB endpoint, and they are hardcoded
  in the code. Even if they are also hardcoded in the vendor kernel of
  the Galaxy SIII (GT-I9300), I've not checked if they could change. For
  that I'd probably need to check the source code of every other
  smartphones or tablets using that protocol and whose modem is
  connected through USB or HSIC (a subset of the USB protocol that
  works without PHY). Could this stay as-is and settings for that could
  probably be implemented later (not necessarily by me) if me or other
  people find cases where that hardcoding doesn't work.

Denis.

Attachment: pgpK8iVTiJ3Xm.pgp
Description: OpenPGP digital signature