Skip to contentSkip to conversation

[PATCH 0/5]

PATCH Superseded Joshua Clayton 2 participants 8 messages Jul 24, 2015 Staging

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Joshua Clayton

author
#0 lore raw

cover [PATCH 0/5]

The main goal of this series is to get rid of a needless and ugly typedef in the rtl8712 wlan driver.

In the course of fixing that, I found a bug thati will can might (at least in theory) lead to a overrun during a memcpy, as well as a duplicate struct. Finally after cleaning up the typedef, I could not bring myself to leave a variable called SupportedRates in the kernel with my name on it.

I have tested this on amd64. cwthe module loads and doesn't explode

Joshua Clayton (5):
staging: rtl8712: fix buggy size calculation
staging: rtl8712: simplify size calculation
staging: rtl8712: remove duplicate struct
staging: rtl8712: remove typedefs
staging: rtl8712: style fix:

 drivers/staging/rtl8712/ieee80211.c           | 22 ++++++-------
 drivers/staging/rtl8712/rtl871x_cmd.c         | 28 +++++-----------
 drivers/staging/rtl8712/rtl871x_cmd.h         |  4 +--
 drivers/staging/rtl8712/rtl871x_event.h       |  2 +-
 drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 33 +++++++++----------
 drivers/staging/rtl8712/rtl871x_mlme.c        | 47 ++++++++++-----------------
 drivers/staging/rtl8712/rtl871x_mlme.h        |  2 +-
 drivers/staging/rtl8712/rtl871x_mp_ioctl.c    |  6 ++--
 drivers/staging/rtl8712/wlan_bssdef.h         | 32 +++---------------
 9 files changed, 63 insertions(+), 113 deletions(-)
Signature · 1 line
2.4.6
author
#1 lore raw

PATCH 1/5 staging: rtl8712: fix buggy size calculation

r8712_get_ndis_wlan_bssid_ex_sz has a "6 * sizeof(unsigned long)" where the underlying struct has a 6 * unsigned char. Simplify the calculation by just subtracting the variable part from the size of the struct.

This also gets rid of a use of typedef NDIS_802_11_RATES_EX

Signed-off-by Joshua Clayton <[email protected]>
---
 drivers/staging/rtl8712/rtl871x_mlme.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)
drivers/staging/rtl8712/rtl871x_mlme.c +1 -11
index c044b0e..6b3451f 100644
@@ -210,17 +210,7 @@ void r8712_generate_random_ibss(u8 *pibss)
 
 uint r8712_get_ndis_wlan_bssid_ex_sz(struct ndis_wlan_bssid_ex *bss)
 {
-	uint t_len;
-
-	t_len = sizeof(u32) + 6 * sizeof(unsigned long) + 2 +
-			sizeof(struct ndis_802_11_ssid) + sizeof(u32) +
-			sizeof(s32) +
-			sizeof(enum NDIS_802_11_NETWORK_TYPE) +
-			sizeof(struct NDIS_802_11_CONFIGURATION) +
-			sizeof(enum NDIS_802_11_NETWORK_INFRASTRUCTURE) +
-			sizeof(NDIS_802_11_RATES_EX) +
-			sizeof(u32) + bss->IELength;
-	return t_len;
+	return sizeof(*bss) + bss->IELength - MAX_IE_SZ;
 }
 
 u8 *r8712_get_capability_from_ie(u8 *ie)
Signature · 1 line
2.4.6
Reviewed-by on v1 1/5
#2 lore raw

Re: [PATCH 1/5] staging: rtl8712: fix buggy size calculation

Quoted 8 lines · Joshua Clayton

r8712_get_ndis_wlan_bssid_ex_sz has a "6 * sizeof(unsigned long)" where the underlying struct has a 6 * unsigned char. Simplify the calculation by just subtracting the variable part from the size of the struct.

This also gets rid of a use of typedef NDIS_802_11_RATES_EX

Signed-off-by Joshua Clayton <[email protected]>

Looks like you are right.

Reviewed-by Dan Carpenter

regards,
dan carpenter

author
#3 lore raw

PATCH 2/5 staging: rtl8712: simplify size calculation

In reply to Joshua Clayton

replace item-by-item size calculation of a struct with the size of the struct.

This gets rid of a use of typedef NDIS_802_11_RATES_EX

Signed-off-by Joshua Clayton <[email protected]>
---
 drivers/staging/rtl8712/rtl871x_cmd.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)
drivers/staging/rtl8712/rtl871x_cmd.c +1 -10
index e35854d..f07050d 100644
@@ -471,7 +471,6 @@ u8 r8712_createbss_cmd(struct _adapter *padapter)
 
 u8 r8712_joinbss_cmd(struct _adapter  *padapter, struct wlan_network *pnetwork)
 {
-	uint t_len = 0;
 	struct ndis_wlan_bssid_ex *psecnetwork;
 	struct cmd_obj		*pcmd;
 	struct cmd_priv		*pcmdpriv = &padapter->cmdpriv;

@@ -486,14 +485,6 @@ u8 r8712_joinbss_cmd(struct _adapter  *padapter, struct wlan_network *pnetwork)
 	pcmd = kmalloc(sizeof(*pcmd), GFP_ATOMIC);
 	if (pcmd == NULL)
 		return _FAIL;
-	t_len = sizeof(u32) + 6 * sizeof(unsigned char) + 2 +
-			sizeof(struct ndis_802_11_ssid) + sizeof(u32) +
-			sizeof(s32) +
-			sizeof(enum NDIS_802_11_NETWORK_TYPE) +
-			sizeof(struct NDIS_802_11_CONFIGURATION) +
-			sizeof(enum NDIS_802_11_NETWORK_INFRASTRUCTURE) +
-			sizeof(NDIS_802_11_RATES_EX) +
-			sizeof(u32) + MAX_IE_SZ;
 
 	/* for hidden ap to set fw_state here */
 	if (check_fwstate(pmlmepriv, WIFI_STATION_STATE|WIFI_ADHOC_STATE) !=

@@ -516,7 +507,7 @@ u8 r8712_joinbss_cmd(struct _adapter  *padapter, struct wlan_network *pnetwork)
 		kfree(pcmd);
 		return _FAIL;
 	}
-	memcpy(psecnetwork, &pnetwork->network, t_len);
+	memcpy(psecnetwork, &pnetwork->network, sizeof(*psecnetwork));
 	psecuritypriv->authenticator_ie[0] = (unsigned char)
 					     psecnetwork->IELength;
 	if ((psecnetwork->IELength-12) < (256 - 1))
Signature · 1 line
2.4.6
author
#4 lore raw

PATCH 4/5 staging: rtl8712: remove typedefs

In reply to Joshua Clayton

Coding style fix.
Get rid of typedefs NDIS_802_11_RATES and NDIS_802_11_RATES_EX

Signed-off-by Joshua Clayton <[email protected]>
---
 drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 4 ++--
 drivers/staging/rtl8712/wlan_bssdef.h         | 7 +------
 2 files changed, 3 insertions(+), 8 deletions(-)
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +2 -2
index 4f5f69c..087fbf1 100644
@@ -635,7 +635,7 @@ static int r8711_wx_get_name(struct net_device *dev,
 	u8 ht_cap = false;
 	struct	mlme_priv	*pmlmepriv = &(padapter->mlmepriv);
 	struct wlan_bssid_ex *pcur_bss = &pmlmepriv->cur_network.network;
-	NDIS_802_11_RATES_EX *prates = NULL;
+	u8 *prates;
 
 	if (check_fwstate(pmlmepriv, _FW_LINKED|WIFI_ADHOC_MASTER_STATE) ==
 	    true) {

@@ -644,7 +644,7 @@ static int r8711_wx_get_name(struct net_device *dev,
 				 &ht_ielen, pcur_bss->IELength - 12);
 		if (p && ht_ielen > 0)
 			ht_cap = true;
-		prates = &pcur_bss->SupportedRates;
+		prates = pcur_bss->SupportedRates;
 		if (r8712_is_cckratesonly_included((u8 *)prates) == true) {
 			if (ht_cap == true)
 				snprintf(wrqu->name, IFNAMSIZ,
drivers/staging/rtl8712/wlan_bssdef.h +1 -6
index 7d769e8..7a410b5 100644
@@ -32,11 +32,6 @@
 #define NDIS_802_11_LENGTH_RATES        8
 #define NDIS_802_11_LENGTH_RATES_EX     16
 
-/* Set of 8 data rates*/
-typedef unsigned char   NDIS_802_11_RATES[NDIS_802_11_LENGTH_RATES];
-/* Set of 16 data rates */
-typedef unsigned char   NDIS_802_11_RATES_EX[NDIS_802_11_LENGTH_RATES_EX];
-
 struct ndis_802_11_ssid {
 	u32 SsidLength;
 	u8  Ssid[32];

@@ -104,7 +99,7 @@ struct wlan_bssid_ex {
 	enum NDIS_802_11_NETWORK_TYPE  NetworkTypeInUse;
 	struct NDIS_802_11_CONFIGURATION  Configuration;
 	enum NDIS_802_11_NETWORK_INFRASTRUCTURE  InfrastructureMode;
-	NDIS_802_11_RATES_EX  SupportedRates;
+	u8 SupportedRates[NDIS_802_11_LENGTH_RATES_EX];
 	u32 IELength;
 	/*(timestamp, beacon interval, and capability information) */
 	u8 IEs[MAX_IE_SZ];
Signature · 1 line
2.4.6
author
#5 lore raw

PATCH 5/5 staging: rtl8712: style fix:

In reply to Joshua Clayton

change instances SupportedRates to compliant and sane "rates" This change in no way harms readability, and brings several lines under the 80 character limit.

Signed-off-by Joshua Clayton <[email protected]>
---
 drivers/staging/rtl8712/ieee80211.c           | 22 +++++++++++-----------
 drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 21 +++++++++------------
 drivers/staging/rtl8712/wlan_bssdef.h         |  4 ++--
 3 files changed, 22 insertions(+), 25 deletions(-)
drivers/staging/rtl8712/ieee80211.c +11 -11
index 5786808..ca9dcaf 100644
@@ -134,22 +134,22 @@ u8 *r8712_get_ie(u8 *pbuf, sint index, sint *len, sint limit)
 	return NULL;
 }
 
-static void set_supported_rate(u8 *SupportedRates, uint mode)
+static void set_supported_rate(u8 *rates, uint mode)
 {
-	memset(SupportedRates, 0, NDIS_802_11_LENGTH_RATES_EX);
+	memset(rates, 0, NDIS_802_11_LENGTH_RATES_EX);
 	switch (mode) {
 	case WIRELESS_11B:
-		memcpy(SupportedRates, WIFI_CCKRATES,
+		memcpy(rates, WIFI_CCKRATES,
 			IEEE80211_CCK_RATE_LEN);
 		break;
 	case WIRELESS_11G:
 	case WIRELESS_11A:
-		memcpy(SupportedRates, WIFI_OFDMRATES,
+		memcpy(rates, WIFI_OFDMRATES,
 			IEEE80211_NUM_OFDM_RATESLEN);
 		break;
 	case WIRELESS_11BG:
-		memcpy(SupportedRates, WIFI_CCKRATES, IEEE80211_CCK_RATE_LEN);
-		memcpy(SupportedRates + IEEE80211_CCK_RATE_LEN, WIFI_OFDMRATES,
+		memcpy(rates, WIFI_CCKRATES, IEEE80211_CCK_RATE_LEN);
+		memcpy(rates + IEEE80211_CCK_RATE_LEN, WIFI_OFDMRATES,
 			IEEE80211_NUM_OFDM_RATESLEN);
 		break;
 	}

@@ -195,17 +195,17 @@ int r8712_generate_ie(struct registry_priv *pregistrypriv)
 	ie = r8712_set_ie(ie, _SSID_IE_, pdev_network->Ssid.SsidLength,
 		    pdev_network->Ssid.Ssid, &sz);
 	/*supported rates*/
-	set_supported_rate(pdev_network->SupportedRates,
+	set_supported_rate(pdev_network->rates,
 			   pregistrypriv->wireless_mode);
-	rateLen = r8712_get_rateset_len(pdev_network->SupportedRates);
+	rateLen = r8712_get_rateset_len(pdev_network->rates);
 	if (rateLen > 8) {
 		ie = r8712_set_ie(ie, _SUPPORTEDRATES_IE_, 8,
-			    pdev_network->SupportedRates, &sz);
+			    pdev_network->rates, &sz);
 		ie = r8712_set_ie(ie, _EXT_SUPPORTEDRATES_IE_, (rateLen - 8),
-			    (pdev_network->SupportedRates + 8), &sz);
+			    (pdev_network->rates + 8), &sz);
 	} else
 		ie = r8712_set_ie(ie, _SUPPORTEDRATES_IE_,
-			    rateLen, pdev_network->SupportedRates, &sz);
+			    rateLen, pdev_network->rates, &sz);
 	/*DS parameter set*/
 	ie = r8712_set_ie(ie, _DSSET_IE_, 1,
 		    (u8 *)&(pdev_network->Configuration.DSConfig), &sz);
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +9 -12
index 087fbf1..219239c 100644
@@ -203,14 +203,12 @@ static inline char *translate_scan(struct _adapter *padapter,
 	}
 	/* Add the protocol name */
 	iwe.cmd = SIOCGIWNAME;
-	if ((r8712_is_cckratesonly_included((u8 *)&pnetwork->network.
-	     SupportedRates)) == true) {
+	if (r8712_is_cckratesonly_included(pnetwork->network.rates)) {
 		if (ht_cap == true)
 			snprintf(iwe.u.name, IFNAMSIZ, "IEEE 802.11bn");
 		else
 			snprintf(iwe.u.name, IFNAMSIZ, "IEEE 802.11b");
-	} else if ((r8712_is_cckrates_included((u8 *)&pnetwork->network.
-		    SupportedRates)) == true) {
+	} else if (r8712_is_cckrates_included(pnetwork->network.rates)) {
 		if (ht_cap == true)
 			snprintf(iwe.u.name, IFNAMSIZ, "IEEE 802.11bgn");
 		else

@@ -270,9 +268,9 @@ static inline char *translate_scan(struct _adapter *padapter,
 	iwe.u.bitrate.disabled = 0;
 	iwe.u.bitrate.value = 0;
 	i = 0;
-	while (pnetwork->network.SupportedRates[i] != 0) {
+	while (pnetwork->network.rates[i] != 0) {
 		/* Bit rate given in 500 kb/s units */
-		iwe.u.bitrate.value = (pnetwork->network.SupportedRates[i++] &
+		iwe.u.bitrate.value = (pnetwork->network.rates[i++] &
 				      0x7F) * 500000;
 		current_val = iwe_stream_add_value(info, start, current_val,
 			      stop, &iwe, IW_EV_PARAM_LEN);

@@ -644,15 +642,15 @@ static int r8711_wx_get_name(struct net_device *dev,
 				 &ht_ielen, pcur_bss->IELength - 12);
 		if (p && ht_ielen > 0)
 			ht_cap = true;
-		prates = pcur_bss->SupportedRates;
-		if (r8712_is_cckratesonly_included((u8 *)prates) == true) {
+		prates = pcur_bss->rates;
+		if (r8712_is_cckratesonly_included(prates) == true) {
 			if (ht_cap == true)
 				snprintf(wrqu->name, IFNAMSIZ,
 					 "IEEE 802.11bn");
 			else
 				snprintf(wrqu->name, IFNAMSIZ,
 					 "IEEE 802.11b");
-		} else if ((r8712_is_cckrates_included((u8 *)prates)) == true) {
+		} else if ((r8712_is_cckrates_included(prates)) == true) {
 			if (ht_cap == true)
 				snprintf(wrqu->name, IFNAMSIZ,
 					 "IEEE 802.11bgn");

@@ -1444,9 +1442,8 @@ static int r8711_wx_get_rate(struct net_device *dev,
 				    (IEEE80211_HT_CAP_SGI_20 |
 				    IEEE80211_HT_CAP_SGI_40)) ? 1 : 0;
 		}
-		while ((pcur_bss->SupportedRates[i] != 0) &&
-			(pcur_bss->SupportedRates[i] != 0xFF)) {
-			rate = pcur_bss->SupportedRates[i] & 0x7F;
+		while (pcur_bss->rates[i] && (pcur_bss->rates[i] != 0xFF)) {
+			rate = pcur_bss->rates[i] & 0x7F;
 			if (rate > max_rate)
 				max_rate = rate;
 			wrqu->bitrate.fixed = 0;	/* no auto select */
drivers/staging/rtl8712/wlan_bssdef.h +2 -2
index 7a410b5..8e2d798 100644
@@ -83,7 +83,7 @@ struct NDIS_802_11_FIXED_IEs {
  * 6 * sizeof (unsigned char) + 2 + sizeof (ndis_802_11_ssid) + sizeof (u32)
  * + sizeof (s32) + sizeof (NDIS_802_11_NETWORK_TYPE)
  * + sizeof (struct NDIS_802_11_CONFIGURATION)
- * + sizeof (NDIS_802_11_RATES_EX) + IELength
+ * + sizeof (rates) + IELength
 
  * Except the IELength, all other fields are fixed length. Therefore, we can
  * define a macro to present the partial sum.

@@ -99,7 +99,7 @@ struct wlan_bssid_ex {
 	enum NDIS_802_11_NETWORK_TYPE  NetworkTypeInUse;
 	struct NDIS_802_11_CONFIGURATION  Configuration;
 	enum NDIS_802_11_NETWORK_INFRASTRUCTURE  InfrastructureMode;
-	u8 SupportedRates[NDIS_802_11_LENGTH_RATES_EX];
+	u8 rates[NDIS_802_11_LENGTH_RATES_EX];
 	u32 IELength;
 	/*(timestamp, beacon interval, and capability information) */
 	u8 IEs[MAX_IE_SZ];
Signature · 1 line
2.4.6
#6 lore raw

Re: [PATCH 5/5] staging: rtl8712: style fix:

Write a better subject line.

Quoted 4 lines · Joshua Clayton

change instances SupportedRates to compliant and sane "rates" This change in no way harms readability, and brings several lines under the 80 character limit.

Yeah, but it does a some other stuff as well like removing casts.

drivers/staging/rtl8712/rtl871x_ioctl_linux.c static int r8711_wx_get_rate(struct net_device *dev, · line 1449

Quoted 8 lines
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +2 -3
-		while ((pcur_bss->SupportedRates[i] != 0) &&
-			(pcur_bss->SupportedRates[i] != 0xFF)) {
-			rate = pcur_bss->SupportedRates[i] & 0x7F;
+		while (pcur_bss->rates[i] && (pcur_bss->rates[i] != 0xFF)) {
+			rate = pcur_bss->rates[i] & 0x7F;
 			if (rate > max_rate)
 				max_rate = rate;
 			wrqu->bitrate.fixed = 0;	/* no auto select */

I actually like the != 0 here because we're talking about the number zero. It should look like this:

		while (pcur_bss->rates[i] != 0 &&
		       pcur_bss->rates[i] != 0xFF) {

But removing the parens is something for a different patch. I use a script to help review these so when you mix different changes together it means there is more manual review work for me.

regards,
dan carpenter

author
#7 lore raw

Re: [PATCH 5/5] staging: rtl8712: style fix:

Dan,

Quoted 8 lines · Dan Carpenter

Write a better subject line.

Quoted 3 lines · Joshua Clayton

change instances SupportedRates to compliant and sane "rates" This change in no way harms readability, and brings several lines under the 80 character limit.

Yeah, but it does a some other stuff as well like removing casts.

I apologize. I thought it would make sense to double up (while I'm in there I'll just...).

Quoted 11 lines
Quoted 11 lines
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +2 -3
-		while ((pcur_bss->SupportedRates[i] != 0) &&
-			(pcur_bss->SupportedRates[i] != 0xFF)) {
-			rate = pcur_bss->SupportedRates[i] & 0x7F;
+		while (pcur_bss->rates[i] && (pcur_bss->rates[i] != 0xFF)) {
+			rate = pcur_bss->rates[i] & 0x7F;
 
 			if (rate > max_rate)
 
 				max_rate = rate;
Quoted 7 lines
Quoted 1 line

wrqu->bitrate.fixed = 0; /* no auto select */

I actually like the != 0 here because we're talking about the number zero. It should look like this:

		while (pcur_bss->rates[i] != 0 &&
		       pcur_bss->rates[i] != 0xFF) {

OK.

Quoted 4 lines

But removing the parens is something for a different patch. I use a script to help review these so when you mix different changes together it means there is more manual review work for me.

Sorry about that.
I'll split or drop it in the next version

Quoted 3 lines

regards,
dan carpenter

Joshua