| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <527550FF.4080506@lwfinger.net>
Date: Sat, 02 Nov 2013 14:22:39 -0500
From: Larry Finger <Larry.Finger@...inger.net>
To: Joe Perches <joe@...ches.com>,
Ben Hutchings <bhutchings@...arflare.com>
CC: linville@...driver.com, linux-wireless@...r.kernel.org,
Mark Cave-Ayland <mark.cave-ayland@...nde.co.uk>,
netdev@...r.kernel.org, Stable <stable@...r.kernel.org>
Subject: Re: [PATCH NEXT V2] rtlwifi: Fix endian error in extracting packet
type
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 <mark.cave-ayland@...nde.co.uk>
>>>
>>> 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 <Larry.Finger@...inger.net>
>>> Cc: Mark Cave-Ayland <mark.cave-ayland@...nde.co.uk>
>>> Cc: Stable <stable@...r.kernel.org> [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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists