On 4 January 2017 at 11:48, Arend Van Spriel
<arend.vanspriel@xxxxxxxxxxxx> wrote:
> On 4-1-2017 11:40, Rafał Miłecki wrote:
>> 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.
>
> Uhm. There is a suggestion above. :-p
... sorry, it seems I should take a break ;)
--
Rafał
