On 4 January 2017 at 10:39, Arend Van Spriel
<arend.vanspriel@xxxxxxxxxxxx> wrote:
> On 3-1-2017 17:49, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> ---
>> 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
>> ---
>> .../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..a16dd7b 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("Firmware reported unexpected channel %d\n",
>> + ch.control_ch_num);
>
> Maybe rephrase to "Ignoring unexpected firmware channel %d\n" so
> end-users are not alarmed by this error message. I think using
> brcmf_err() is justified, but you may even consider chiming down to
> brcmf_dbg(INFO, ...).
Can you suggest a better error message? It seems I'm too brave and I
don't find this one alarming, so I need suggestion.
--
Rafał
