Re: [PATCH next V2] brcmfmac: avoid writing channel out of allocated array

[Arend Van Spriel <arend.vanspriel@broadcom.com>]

On 4-1-2017 11:40, Rafał Miłecki wrote:
> On 4 January 2017 at 10:39, Arend Van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>> On 3-1-2017 17:49, 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>
>>> ---
>>> 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