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
Regards,
Arend
