| Message ID | 20170104110941.21261-1-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 77c0d0cd10e793989d1e8b835a9a09694182cb39 |
| Delegated to: | Kalle Valo |
| Headers | show |
On 4-1-2017 12:09, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> Not taking a break? > 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") Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com> > 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; > } > } >
Rafał Miłecki wrote: > 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> > Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com> Patch applied to wireless-drivers-next.git, thanks. 77c0d0cd10e7 brcmfmac: avoid writing channel out of allocated array
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; } }