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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 1 Nov 2013 01:02:49 +0000
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Larry Finger <Larry.Finger@...inger.net>
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 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)) ||
			     ...

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
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