[PATCH] brcmsmac: fix NULL pointer crash in brcms_c_regd_init()

[PATCH] brcmsmac: fix NULL pointer crash in brcms_c_regd_init()

[Arend van Spriel]

In the function brcms_c_regd_init() the channels are validated
against the device capabilities. This is done for both 2.4G and
5G band, but there are devices that are 2.4G only, ie. BCM4313.
For that device this leads to a NULL dereference. This patch adds
a check in brcms_c_regd_init() to fix this.

Issue introduced in wireless-next tree by following commit:
cf03c5d brcm80211: smac: inform mac80211 of the X2 regulatory domain

Cc: Seth Forshee <seth.forshee@canonical.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
Noticed that one of smoketest machines actually was not sending results
ever since I staged regulatory fixes for it. This particular system has
a 2.4G card fitted so the root cause was quickly found.

Gr. AvS
---
 drivers/net/wireless/brcm80211/brcmsmac/channel.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/brcm80211/brcmsmac/channel.c b/drivers/net/wireless/brcm80211/brcmsmac/channel.c
index d3c7260..2d365d3 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/channel.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/channel.c
@@ -768,6 +768,11 @@ void brcms_c_regd_init(struct brcms_c_info *wlc)
 			band = wlc->bandstate[BAND_2G_INDEX];
 		else
 			band = wlc->bandstate[BAND_5G_INDEX];
+
+		/* skip if band not initialized */
+		if (band->pi == NULL)
+			continue;
+
 		wlc_phy_chanspec_band_validch(band->pi, band->bandtype,
 					      &sup_chan);
 
-- 
1.7.9.5

Re: [PATCH] brcmsmac: fix NULL pointer crash in brcms_c_regd_init()

[Seth Forshee]

Hi Arend,

Sorry for my slow response. I was on vacation last week.

On Thu, Jun 21, 2012 at 03:49:13PM +0200, Arend van Spriel wrote:
> In the function brcms_c_regd_init() the channels are validated
> against the device capabilities. This is done for both 2.4G and
> 5G band, but there are devices that are 2.4G only, ie. BCM4313.
> For that device this leads to a NULL dereference. This patch adds
> a check in brcms_c_regd_init() to fix this.
> 
> Issue introduced in wireless-next tree by following commit:
> cf03c5d brcm80211: smac: inform mac80211 of the X2 regulatory domain
> 
> Cc: Seth Forshee <seth.forshee@canonical.com>
> Signed-off-by: Arend van Spriel <arend@broadcom.com>
> ---
> Noticed that one of smoketest machines actually was not sending results
> ever since I staged regulatory fixes for it. This particular system has
> a 2.4G card fitted so the root cause was quickly found.
> 
> Gr. AvS
> ---
>  drivers/net/wireless/brcm80211/brcmsmac/channel.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/wireless/brcm80211/brcmsmac/channel.c b/drivers/net/wireless/brcm80211/brcmsmac/channel.c
> index d3c7260..2d365d3 100644
> --- a/drivers/net/wireless/brcm80211/brcmsmac/channel.c
> +++ b/drivers/net/wireless/brcm80211/brcmsmac/channel.c
> @@ -768,6 +768,11 @@ void brcms_c_regd_init(struct brcms_c_info *wlc)
>  			band = wlc->bandstate[BAND_2G_INDEX];
>  		else
>  			band = wlc->bandstate[BAND_5G_INDEX];
> +
> +		/* skip if band not initialized */
> +		if (band->pi == NULL)
> +			continue;
> +

Yeah, I definitely messed that up.

This looks fine, but it strikes me that it might simplify things a bit
to change the loop to iterate over over wlc->pub->_nbands instead. The
difference is pretty minor though, and since John has already applied
this patch there's probably no reason to change it.

Thanks,
Seth

Re: [PATCH] brcmsmac: fix NULL pointer crash in brcms_c_regd_init()

[Arend van Spriel]

On 06/25/2012 06:53 PM, Seth Forshee wrote:
> Yeah, I definitely messed that up.

I have my off days as well. We are still human, right ;-)

> This looks fine, but it strikes me that it might simplify things a bit
> to change the loop to iterate over over wlc->pub->_nbands instead. The
> difference is pretty minor though, and since John has already applied
> this patch there's probably no reason to change it.

I considered that but the iterator is used as index in the wiphy.band[]
array further in the loop. So that would require a bit more rework of
the loop (although the internal band index and the one from cfg80211 match).

Gr. AvS

Re: [PATCH] brcmsmac: fix NULL pointer crash in brcms_c_regd_init()

[Seth Forshee]

On Tue, Jun 26, 2012 at 11:46:33AM +0200, Arend van Spriel wrote:
> > This looks fine, but it strikes me that it might simplify things a bit
> > to change the loop to iterate over over wlc->pub->_nbands instead. The
> > difference is pretty minor though, and since John has already applied
> > this patch there's probably no reason to change it.
> 
> I considered that but the iterator is used as index in the wiphy.band[]
> array further in the loop. So that would require a bit more rework of
> the loop

It's really not that bad though. I pasted an (untested) patch below.
But like I said before, the difference is pretty minor so I'm okay with
either.

> (although the internal band index and the one from cfg80211 match).

While this is true, I wouldn't want to build that assumption into the
code. Maybe there should be macros to map between the internal and
cfg80211 band indices.


diff --git a/drivers/net/wireless/brcm80211/brcmsmac/channel.c b/drivers/net/wireless/brcm80211/brcmsmac/channel.c
index 2d365d3..2c10bbe 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/channel.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/channel.c
@@ -760,23 +760,17 @@ void brcms_c_regd_init(struct brcms_c_info *wlc)
 	struct ieee80211_channel *ch;
 	struct brcms_chanvec sup_chan;
 	struct brcms_band *band;
-	int band_idx, i;
+	uint band_idx, i;
 
 	/* Disable any channels not supported by the phy */
-	for (band_idx = 0; band_idx < IEEE80211_NUM_BANDS; band_idx++) {
-		if (band_idx == IEEE80211_BAND_2GHZ)
-			band = wlc->bandstate[BAND_2G_INDEX];
-		else
-			band = wlc->bandstate[BAND_5G_INDEX];
-
-		/* skip if band not initialized */
-		if (band->pi == NULL)
-			continue;
+	for (band_idx = 0; band_idx < wlc->pub->_nbands; band_idx++) {
+		band = wlc->bandstate[band_idx];
+		sband = wiphy->bands[band_idx == BAND_2G_INDEX ?
+				     IEEE80211_BAND_2GHZ : IEEE80211_BAND_5GHZ];
 
 		wlc_phy_chanspec_band_validch(band->pi, band->bandtype,
 					      &sup_chan);
 
-		sband = wiphy->bands[band_idx];
 		for (i = 0; i < sband->n_channels; i++) {
 			ch = &sband->channels[i];
 			if (!isset(sup_chan.vec, ch->hw_value))