All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Rafał Miłecki" <zajec5@gmail.com>
To: Kalle Valo <kvalo@codeaurora.org>
Cc: "Arend van Spriel" <arend.vanspriel@broadcom.com>,
	"Franky Lin" <franky.lin@broadcom.com>,
	"Hante Meuleman" <hante.meuleman@broadcom.com>,
	"Pieter-Paul Giesberts" <pieter-paul.giesberts@broadcom.com>,
	"Franky Lin" <frankyl@broadcom.com>,
	linux-wireless@vger.kernel.org,
	brcm80211-dev-list.pdl@broadcom.com,
	"Rafał Miłecki" <rafal@milecki.pl>
Subject: [PATCH V3] brcmfmac: avoid writing channel out of allocated array
Date: Wed,  4 Jan 2017 12:09:41 +0100	[thread overview]
Message-ID: <20170104110941.21261-1-zajec5@gmail.com> (raw)
In-Reply-To: <20170103164930.29989-1-zajec5@gmail.com>

From: Rafał Miłecki <rafal@milecki.pl>

Our code was assigning number of channels to the index variable by
default. If firmware reported channel we didn't predict this would
result in using that initial index value and writing out of array. This
never happened so far (we got a complete list of supported channels) but
it means possible memory corruption so we should handle it anyway.

This patch simply detects unexpected channel and ignores it.

As we don't try to create new entry now, it's also safe to drop hw_value
and center_freq assignment. For known channels we have these set anyway.

I decided to fix this issue by assigning NULL or a target channel to the
channel variable. This was one of possible ways, I prefefred this one as
it also avoids using channel[index] over and over.

Fixes: 58de92d2f95e ("brcmfmac: use static superset of channels for wiphy bands")
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Add extra comment in code for not-found channel.
    Make it clear this problem have never been seen so far
    Explain why it's safe to drop extra assignments
    Note & reason changing channel variable usage
V3: Update error message as suggested by Arend.
---
 .../broadcom/brcm80211/brcmfmac/cfg80211.c         | 32 ++++++++++++----------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 9c2c128..75ef23b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -5825,7 +5825,6 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
 	u32 i, j;
 	u32 total;
 	u32 chaninfo;
-	u32 index;
 
 	pbuf = kzalloc(BRCMF_DCMD_MEDLEN, GFP_KERNEL);
 
@@ -5873,33 +5872,36 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
 		    ch.bw == BRCMU_CHAN_BW_80)
 			continue;
 
-		channel = band->channels;
-		index = band->n_channels;
+		channel = NULL;
 		for (j = 0; j < band->n_channels; j++) {
-			if (channel[j].hw_value == ch.control_ch_num) {
-				index = j;
+			if (band->channels[j].hw_value == ch.control_ch_num) {
+				channel = &band->channels[j];
 				break;
 			}
 		}
-		channel[index].center_freq =
-			ieee80211_channel_to_frequency(ch.control_ch_num,
-						       band->band);
-		channel[index].hw_value = ch.control_ch_num;
+		if (!channel) {
+			/* It seems firmware supports some channel we never
+			 * considered. Something new in IEEE standard?
+			 */
+			brcmf_err("Ignoring unexpected firmware channel %d\n",
+				  ch.control_ch_num);
+			continue;
+		}
 
 		/* assuming the chanspecs order is HT20,
 		 * HT40 upper, HT40 lower, and VHT80.
 		 */
 		if (ch.bw == BRCMU_CHAN_BW_80) {
-			channel[index].flags &= ~IEEE80211_CHAN_NO_80MHZ;
+			channel->flags &= ~IEEE80211_CHAN_NO_80MHZ;
 		} else if (ch.bw == BRCMU_CHAN_BW_40) {
-			brcmf_update_bw40_channel_flag(&channel[index], &ch);
+			brcmf_update_bw40_channel_flag(channel, &ch);
 		} else {
 			/* enable the channel and disable other bandwidths
 			 * for now as mentioned order assure they are enabled
 			 * for subsequent chanspecs.
 			 */
-			channel[index].flags = IEEE80211_CHAN_NO_HT40 |
-					       IEEE80211_CHAN_NO_80MHZ;
+			channel->flags = IEEE80211_CHAN_NO_HT40 |
+					 IEEE80211_CHAN_NO_80MHZ;
 			ch.bw = BRCMU_CHAN_BW_20;
 			cfg->d11inf.encchspec(&ch);
 			chaninfo = ch.chspec;
@@ -5907,11 +5909,11 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
 						       &chaninfo);
 			if (!err) {
 				if (chaninfo & WL_CHAN_RADAR)
-					channel[index].flags |=
+					channel->flags |=
 						(IEEE80211_CHAN_RADAR |
 						 IEEE80211_CHAN_NO_IR);
 				if (chaninfo & WL_CHAN_PASSIVE)
-					channel[index].flags |=
+					channel->flags |=
 						IEEE80211_CHAN_NO_IR;
 			}
 		}
-- 
2.10.1

  parent reply	other threads:[~2017-01-04 11:09 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-01-03  8:38 [PATCH] brcmfmac: avoid writing channel out of allocated array Rafał Miłecki
2017-01-03 11:02 ` Arend Van Spriel
2017-01-03 11:31   ` Rafał Miłecki
2017-01-03 13:19     ` Arend Van Spriel
2017-01-03 14:14       ` Rafał Miłecki
2017-01-03 15:49         ` Rafał Miłecki
2017-01-04  8:12           ` Kalle Valo
2017-01-04  8:14     ` Kalle Valo
2017-01-04  8:08   ` Kalle Valo
2017-01-03 16:49 ` [PATCH next V2] " Rafał Miłecki
2017-01-04  9:39   ` Arend Van Spriel
2017-01-04 10:40     ` Rafał Miłecki
2017-01-04 10:48       ` Arend Van Spriel
2017-01-04 11:04         ` Rafał Miłecki
2017-01-04 11:09   ` Rafał Miłecki [this message]
2017-01-04 11:11     ` [PATCH V3] " Arend Van Spriel
2017-01-17 11:57     ` [V3] " Kalle Valo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170104110941.21261-1-zajec5@gmail.com \
    --to=zajec5@gmail.com \
    --cc=arend.vanspriel@broadcom.com \
    --cc=brcm80211-dev-list.pdl@broadcom.com \
    --cc=franky.lin@broadcom.com \
    --cc=frankyl@broadcom.com \
    --cc=hante.meuleman@broadcom.com \
    --cc=kvalo@codeaurora.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=pieter-paul.giesberts@broadcom.com \
    --cc=rafal@milecki.pl \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.