Subject: Re: [PATCH NEXT V2] rtlwifi: Fix endian error in extracting packet type

From: Larry Finger <[email-masked]>
Date: Sat, 02 Nov 2013 14:22:39 -0500

On 10/31/2013 08:48 PM, Joe Perches wrote:
> On Fri, 2013-11-01 at 01:02 +0000, Ben Hutchings wrote:
>> On Thu, 2013-10-31 at 19:43 -0500, Larry Finger wrote:
>>> From: Mark Cave-Ayland <[email-masked]>
>>>
>>> All of the rtlwifi drivers have an error in the routine that tests if
>>> the data is "special". If it is, the subsequant transmission will be
>>> at the lowest rate to enhance reliability. The 16-bit quantity is
>>> big-endian, but was being extracted in native CPU mode. One of the
>>> effects of this bug is to inhibit association under some conditions
>>> as the TX rate is too high.
>>>
>>> A statement that would have made the code correct had been changed to
>>> a comment. Rather than just reinstating that code, the fix here passes
>>> sparse tests. A side effect of fixing this problem would have been to force
>>> all IPv6 frames to run at the lowest rate. The test for that frame type
>>> is removed.
>>>
>>> The original code only checked the lower-order byte of UDP ports for BOOTP
>>> protocol. That is extended to the full 16-bit source and destination ports.
>>>
>>> One of the local headers contained duplicates of some of the ETH_P_XXX
>>> definitions. These are deleted.
>>>
>>> Signed-off-by: Larry Finger <[email-masked]>
>>> Cc: Mark Cave-Ayland <[email-masked]>
>>> Cc: Stable <[email-masked]> [2.6.38+]
>>> ---
>>>
>>> V2 - Addresses comments by Ben Hutchings and Bjorn Mork.
>>>
>>>   drivers/net/wireless/rtlwifi/base.c |   15 ++++++---------
>>>   drivers/net/wireless/rtlwifi/wifi.h |    6 +-----
>>>   2 files changed, 7 insertions(+), 14 deletions(-)
>>> Index: wireless-testing-save/drivers/net/wireless/rtlwifi/base.c
>>> ===================================================================
>>> --- wireless-testing-save.orig/drivers/net/wireless/rtlwifi/base.c
>>> +++ wireless-testing-save/drivers/net/wireless/rtlwifi/base.c
>>> @@ -1077,17 +1077,17 @@ u8 rtl_is_special_data(struct ieee80211_
>>>
>>>   	ip = (struct iphdr *)((u8 *) skb->data + mac_hdr_len +
>>>   			      SNAP_SIZE + PROTOC_TYPE_SIZE);
>>> -	ether_type = *(u16 *) ((u8 *) skb->data + mac_hdr_len + SNAP_SIZE);
>>> -	/*	ether_type = ntohs(ether_type); */
>>> +	ether_type = be16_to_cpu(*(__be16 *)((u8 *)skb->data + mac_hdr_len +
>>> +					     SNAP_SIZE));
>>>
>>>   	if (ETH_P_IP == ether_type) {
>>>   		if (IPPROTO_UDP == ip->protocol) {
>>>   			struct udphdr *udp = (struct udphdr *)((u8 *) ip +
>>>   							       (ip->ihl << 2));
>>> -			if (((((u8 *) udp)[1] == 68) &&
>>> -			     (((u8 *) udp)[3] == 67)) ||
>>> -			    ((((u8 *) udp)[1] == 67) &&
>>> -			     (((u8 *) udp)[3] == 68))) {
>>> +			if (((((u16 *) udp)[0] == 68) &&
>>> +			     (((u16 *) udp)[2] == 67)) ||
>>> +			    ((((u16 *) udp)[0] == 67) &&
>>> +			     (((u16 *) udp)[2] == 68))) {
>> [...]
>>
>> Now you're missing byte-swapping here, and using the wrong offset for
>> the dest port (4 bytes rather than 2).
>>
>> If you really think this is necessary then use something like:
>> 			if ((udp->source == htons(68) &&
>> 			     udp->dest == htons(67)) ||
>> 			     ...
>
> Or maybe something like this?
> ---
>   drivers/net/wireless/rtlwifi/base.c | 91 +++++++++++++++++--------------------
>   1 file changed, 41 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/net/wireless/rtlwifi/base.c b/drivers/net/wireless/rtlwifi/base.c
> index 9a78e3d..7e9df65 100644
> --- a/drivers/net/wireless/rtlwifi/base.c
> +++ b/drivers/net/wireless/rtlwifi/base.c
> @@ -37,6 +37,7 @@
>
>   #include <linux/ip.h>
>   #include <linux/module.h>
> +#include <linux/udp.h>
>
>   /*
>    *NOTICE!!!: This file will be very big, we should
> @@ -1074,64 +1075,54 @@ u8 rtl_is_special_data(struct ieee80211_hw *hw, struct sk_buff *skb, u8 is_tx)
>   	if (!ieee80211_is_data(fc))
>   		return false;
>
> +	ip = (const struct iphdr *)(skb->data + mac_hdr_len +
> +				    SNAP_SIZE + PROTOC_TYPE_SIZE);
> +	ether_type = be16_to_cpup((__be16 *)
> +				  (skb->data + mac_hdr_len + SNAP_SIZE));
>
> -	ip = (struct iphdr *)((u8 *) skb->data + mac_hdr_len +
> -			      SNAP_SIZE + PROTOC_TYPE_SIZE);
> -	ether_type = *(u16 *) ((u8 *) skb->data + mac_hdr_len + SNAP_SIZE);
> -	/*	ether_type = ntohs(ether_type); */
> -
> -	if (ETH_P_IP == ether_type) {
> -		if (IPPROTO_UDP == ip->protocol) {
> -			struct udphdr *udp = (struct udphdr *)((u8 *) ip +
> -							       (ip->ihl << 2));
> -			if (((((u8 *) udp)[1] == 68) &&
> -			     (((u8 *) udp)[3] == 67)) ||
> -			    ((((u8 *) udp)[1] == 67) &&
> -			     (((u8 *) udp)[3] == 68))) {
> -				/*
> -				 * 68 : UDP BOOTP client
> -				 * 67 : UDP BOOTP server
> -				 */
> -				RT_TRACE(rtlpriv, (COMP_SEND | COMP_RECV),
> -					 DBG_DMESG, "dhcp %s !!\n",
> -					 is_tx ? "Tx" : "Rx");
> -
> -				if (is_tx) {
> -					rtlpriv->enter_ps = false;
> -					schedule_work(&rtlpriv->
> -						      works.lps_change_work);
> -					ppsc->last_delaylps_stamp_jiffies =
> -					    jiffies;
> -				}
> +	switch (ether_type) {
> +	case ETH_P_IP: {
> +		struct udphdr *udp;
> +		u16 src;
> +		u16 dst;
>
> -				return true;
> -			}
> -		}
> -	} else if (ETH_P_ARP == ether_type) {
> -		if (is_tx) {
> -			rtlpriv->enter_ps = false;
> -			schedule_work(&rtlpriv->works.lps_change_work);
> -			ppsc->last_delaylps_stamp_jiffies = jiffies;
> -		}
> +		if (ip->protocol != IPPROTO_UDP)
> +			return false;
>
> -		return true;
> -	} else if (ETH_P_PAE == ether_type) {
> -		RT_TRACE(rtlpriv, (COMP_SEND | COMP_RECV), DBG_DMESG,
> -			 "802.1X %s EAPOL pkt!!\n", is_tx ? "Tx" : "Rx");
> +		udp = (struct udphdr *)((u8 *)ip + (ip->ihl << 2));
> +		src = be16_to_cpu(udp->source) >> 8;
> +		dst = be16_to_cpu(udp->dest) >> 8;
>
> -		if (is_tx) {
> -			rtlpriv->enter_ps = false;
> -			schedule_work(&rtlpriv->works.lps_change_work);
> -			ppsc->last_delaylps_stamp_jiffies = jiffies;
> -		}
> +		/*
> +		 * 68 : UDP BOOTP client
> +		 * 67 : UDP BOOTP server
> +		 */
> +		if (!((src == 68 && dst == 67) || (src == 67 && dst == 68)))
> +			return false;
>
> +		RT_TRACE(rtlpriv, (COMP_SEND | COMP_RECV), DBG_DMESG,
> +			 "dhcp %s !!\n", is_tx ? "Tx" : "Rx");
> +		break;
> +	}
> +	case ETH_P_ARP:
> +		break;
> +	case ETH_P_PAE:
> +		RT_TRACE(rtlpriv, (COMP_SEND | COMP_RECV), DBG_DMESG,
> +			 "802.1X %s EAPOL pkt!!\n", is_tx ? "Tx" : "Rx");
> +		break;
> +	case ETH_P_IPV6:
>   		return true;
> -	} else if (ETH_P_IPV6 == ether_type) {
> -		/* IPv6 */
> -		return true;
> +	default:
> +		return false;
>   	}
>
> -	return false;
> +	if (is_tx) {
> +		rtlpriv->enter_ps = false;
> +		schedule_work(&rtlpriv->works.lps_change_work);
> +		ppsc->last_delaylps_stamp_jiffies = jiffies;
> +	}
> +
> +	return true;
>   }
>   EXPORT_SYMBOL_GPL(rtl_is_special_data);

Joe,

Thanks for this. I have rewritten the function somewhat along these lines. It is 
much cleaner this way.

Larry