[Click] Four small wifi/radiotap-related bug fixes

[Ian Rose]

Howdy again,

I have been messing about with the Wifi- and Radiotap-related elements 
and have come across a few bugs (well, in fairness, #2 is more of a 
missing feature).  Also note that my packet-capture source is the 
FreeBSD ath driver whereas (I assume) most wireless Click users use 
MadWifi on Linux so some of these bugs might be hard or impossible to 
recreate on Linux.

1) WifiDecap does not properly handle cases where padding has been 
inserted after the 802.11 header (so as to word-align the next header).

2) After stripping the 802.11 header, WifiDecap always creates and 
pushes on an Ethernet header; a configuration parameter should exist to 
prevent this from occurring if so desired.

3) RadiotapDecap does not correctly read the "frame includes FCS" flag 
from radiotap headers, meaning packets are not stripped of their FCS (at 
the end of the packet) as intended.

4) RadiotapDecap does not re-set the mac-header pointer 
(p->set_mac_header) after stripping the radiotap header.

To recreate bug #1, I have uploaded a pcap file (consisting of just 1 
packet) here: http://www.eecs.harvard.edu/~ianrose/stuff/wifidecaptest.pcap

The packet's 802.11 header is padded to bring its length up to a 
multiple of 4 bytes.  When viewed with tcpdump (or wireshark, etc.) this 
packet is properly parsed and displayed as a UDP DNS request.  However 
an error is output (bad IP version) when run through the following click 
configuration because it does not handle the padding correctly:

> define($FILENAME wifidecaptest.pcap)
> 
> FromDump($FILENAME, STOP true)
>     -> RadiotapDecap
>     -> WifiDecap
>     -> CheckIPHeader(14, VERBOSE true)
>     -> IPPrint
>     -> Discard;

For "bug" #2, I have added an optional 'PUSH_ETH' configuration 
parameter to the WifiDecap element; its default value is true to 
maintain backwards compatibility.

I identified bugs #3 and #4 by code inspection alone (as opposed to 
actual testing) and I do not have an examples on hand that demonstrate 
either the bugs or the fixes.

I have included candidate patches that address all 4 of these issues below.

Cheers,
Ian

----------------------------------------------------

> --- click-1.7.0rc1/elements/wifi/radiotapdecap.cc	2009-08-06 17:32:06.518608000 -0400
> @@ -122,8 +122,19 @@
>  	struct ieee80211_radiotap_header *th = (struct ieee80211_radiotap_header *) p->data();
>  	struct click_wifi_extra *ceh = WIFI_EXTRA_ANNO(p);
>  	if (rt_check_header(th, p->length())) {
> +		memset((void*)ceh, 0, sizeof(struct click_wifi_extra));
>  		ceh->magic = WIFI_EXTRA_MAGIC;
>  
> +		if (rt_el_present(th, IEEE80211_RADIOTAP_FLAGS)) {
> +			u_int8_t flags = *((u_int8_t *) rt_el_offset(th, IEEE80211_RADIOTAP_FLAGS));
> +			if (flags & IEEE80211_RADIOTAP_F_DATAPAD) {
> +				ceh->pad = 1;
> +			}
> +			if (flags & IEEE80211_RADIOTAP_F_FCS) {
> +				p->take(4);
> +			}
> +		}
> +
>  		if (rt_el_present(th, IEEE80211_RADIOTAP_RATE)) {
>  			ceh->rate = *((u_int8_t *) rt_el_offset(th, IEEE80211_RADIOTAP_RATE));
>  		}
> @@ -151,16 +162,13 @@
>  			ceh->flags |= WIFI_EXTRA_TX;
>  			if (flags & IEEE80211_RADIOTAP_F_TX_FAIL)
>  				ceh->flags |= WIFI_EXTRA_TX_FAIL;
> -
> -			if (flags & IEEE80211_RADIOTAP_F_FCS) {
> -				p->take(4);
> -			}
>  		}
>  
>  		if (rt_el_present(th, IEEE80211_RADIOTAP_DATA_RETRIES))
>  			ceh->retries = *((u_int8_t *) rt_el_offset(th, IEEE80211_RADIOTAP_DATA_RETRIES));
>  
>  		p->pull(th->it_len);
> +		p->set_mac_header(p->data());  // reset mac-header pointer
>  	}
>  
>    return p;

> +++ click-1.7.0rc1/elements/wifi/wifidecap.cc	2009-08-06 17:08:46.592049000 -0400
> @@ -21,6 +21,7 @@
>  #include <click/confparse.hh>
>  #include <click/error.hh>
>  #include <click/glue.hh>
> +#include <click/packet_anno.hh>
>  #include <clicknet/wifi.h>
>  #include <clicknet/llc.h>
>  CLICK_DECLS
> @@ -39,9 +40,11 @@
>  
>    _debug = false;
>    _strict = false;
> +  _push_eth = true;
>    if (cp_va_kparse(conf, this, errh,
>  		   "DEBUG", 0, cpBool, &_debug,
>  		   "STRICT", 0, cpBool, &_strict,
> +		   "PUSH_ETH", 0, cpBool, &_push_eth,
>  		   cpEnd) < 0)
>      return -1;
>    return 0;
> @@ -64,6 +67,10 @@
>    if (WIFI_QOS_HAS_SEQ(w))
>  	  wifi_header_size += sizeof(uint16_t);
>  
> +  struct click_wifi_extra *ceh = WIFI_EXTRA_ANNO(p);
> +  if ((ceh->magic == WIFI_EXTRA_MAGIC) && ceh->pad && (wifi_header_size & 3))
> +	  wifi_header_size += 4 - (wifi_header_size & 3);
> +
>    if (p->length() < wifi_header_size + sizeof(struct click_llc)) {
>      p->kill();
>      return 0;
> @@ -125,23 +132,26 @@
>    }
>  
>    p_out->pull(wifi_header_size + sizeof(struct click_llc));
> -  p_out = p_out->push_mac_header(14);
> -  if (!p_out) {
> -    return 0;
> -  }
>  
> -  memcpy(p_out->data(), dst.data(), 6);
> -  memcpy(p_out->data() + 6, src.data(), 6);
> -  memcpy(p_out->data() + 12, &ether_type, 2);
> -
> -  if (_debug) {
> -	  click_chatter("%{element}: dir %d src %s dst %s bssid %s eth 0x%02x\n",
> -			this,
> -			dir,
> -			src.unparse().c_str(),
> -			dst.unparse().c_str(),
> -			bssid.unparse().c_str(),
> -			ether_type);
> +  if (_push_eth) {
> +	  p_out = p_out->push_mac_header(14);
> +	  if (!p_out) {
> +	    return 0;
> +	  }
> +
> +	  memcpy(p_out->data(), dst.data(), 6);
> +	  memcpy(p_out->data() + 6, src.data(), 6);
> +	  memcpy(p_out->data() + 12, &ether_type, 2);
> +
> +	  if (_debug) {
> +		  click_chatter("%{element}: dir %d src %s dst %s bssid %s eth 0x%02x\n",
> +				this,
> +				dir,
> +				src.unparse().c_str(),
> +				dst.unparse().c_str(),
> +				bssid.unparse().c_str(),
> +				ether_type);
> +	  }
>    }
>  
>    return p_out;

> --- click-1.7.0rc1/elements/wifi/wifidecap.hh	2009-08-06 17:47:45.706426000 -0400
> @@ -53,6 +53,7 @@
>  
>    bool _debug;
>    bool _strict;
> +  bool _push_eth;
>   private:
>  
>  };