Re: [B.A.T.M.A.N.] [PATCHv5] batctl: tcpdump - parse TVLV containers

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 2178 bytes --]

> +typedef void (*batctl_tvlv_parser_t)(void *buff, ssize_t buff_len);
> +
> +/* location [i][j] contains the parsing function for TVLV of type 'i' and
> + * version 'j + 1'
> + */
> +batctl_tvlv_parser_t tvlv_parsers[][1] = {
> +       [BATADV_TVLV_GW][0] = batctl_tvlv_parse_gw_v1,
> +       [BATADV_TVLV_DAT][0] = batctl_tvlv_parse_dat_v1,
> +       [BATADV_TVLV_NC][0] = batctl_tvlv_parse_nc_v1,
> +       [BATADV_TVLV_TT][0] = batctl_tvlv_parse_tt_v1,
> +       [BATADV_TVLV_ROAM][0] = batctl_tvlv_parse_roam_v1,
> +};
[....]
> +       while (tvlv_len > 0) {
> +               tvlv_hdr = (struct batadv_tvlv_hdr *)ptr;
> +               len = ntohs(tvlv_hdr->len);
> +
> +               parser = tvlv_parsers[tvlv_hdr->type][tvlv_hdr->version - 1];
> +               parser(tvlv_hdr + 1, len);
> +
> +               /* go to the next container */
> +               ptr = (uint8_t *)(tvlv_hdr + 1) + len;
> +               tvlv_len -= sizeof(*tvlv_hdr) + len;
> +       }
> +}
> +
[....]
> +       while (tvlv_len > 0) {
> +               tvlv_hdr = (struct batadv_tvlv_hdr *)ptr;
> +               len = ntohs(tvlv_hdr->len);
> +
> +               parser = tvlv_parsers[tvlv_hdr->type][tvlv_hdr->version - 1];
> +               parser(tvlv_hdr + 1, len);
> +
> +               /* go to the next container */
> +               ptr = (uint8_t *)(tvlv_hdr + 1) + len;
> +               tvlv_len -= sizeof(*tvlv_hdr) + len;
> +       }
>  }

I've already explained this to Antonio but here again in public:

 * neither type nor version are validated
 * the type can point in tvlv_parsers  to an invalid (non-existing) entry
 * the version of this entry can also point to an invalid parser
 * this is a big problem because I can crash batctl td with data packets
   send from other people (even valid data packets with mcast tvlv)
 * I've already experienced this problem when using nodes from a company
   which ships this patch since a while (> 4 months)

Maybe it can be redone with some switch statements or
(more space consuming) full tables which can handle all input data. Of course
a check of the returned parser is still necessary inside the loops.

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]