On 3 January 2017 at 12:02, Arend Van Spriel
<arend.vanspriel@xxxxxxxxxxxx> wrote:
> On 3-1-2017 9:38, 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.
>>
>> Fix this by detecting unexpected channel and ignoring it.
>>
>> Fixes: 58de92d2f95e ("brcmfmac: use static superset of channels for wiphy bands")
>> Signed-off-by: Rafał Miłecki <rafal@xxxxxxxxxx>
>> ---
>> I'm not sure what kind of material it is. It fixes possible memory corruption
>> (serious thing?) but this bug was there since Apr 2015, so is it worth fixing
>> in 4.10? Or maybe I should even cc stable?
>> I don't think any released firmware reports any unexpected channel, so I guess
>> noone ever hit this problem. I just noticed this possible problem when working
>> on another feature.
>
> Looking at the change I was going to ask if you actually hit the issue
> you are addressing here. The channels in __wl_2ghz_channels and
> __wl_5ghz_channels are complete list of channels for the particular band
> so it would mean firmware behaves out-of-spec or firmware api was
> changed. For robustness a change is acceptable I guess.
I guess that point of view depends on one's trust to the firmware. I
think it's wrong if a wrong/bugged/hacked firmware can result in
memory corruption in the kernel. Even if it's only about sizeof(struct
ieee80211_channel).
> My general policy is to submit fixes to wireless-drivers (and stable)
> only if it resolves a critical issue found during testing or a reported
> issue.
I'm OK with that.
>> ---
>> .../broadcom/brcm80211/brcmfmac/cfg80211.c | 29 +++++++++++-----------
>> 1 file changed, 14 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> index 13ca3eb..0babfc7 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,33 @@ 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;
>> }
>> }
>
> You could have kept the index construct and simply check if j ==
> band->n_channels here to determine something is wrong.
I wanted to simplify code at the same time. Having channel[index]
repeated 7 times was a hint for me it could be handled better. I
should have made that clear, I'll fix improve this in V2.
>> - channel[index].center_freq =
>> - ieee80211_channel_to_frequency(ch.control_ch_num,
>> - band->band);
>> - channel[index].hw_value = ch.control_ch_num;
>
> So you are also removing these assignments which indeed seem redundant.
> Maybe make note of that in the commit message?
Good idea.
>> + if (!channel) {
>> + brcmf_err("Firmware reported unexpected channel %d\n",
>> + ch.control_ch_num);
>> + continue;
>> + }
>
> As stated above something is really off when this happens so should we
> continue and try to make sense of what firmware provides or simply fail.
Well, I could image something like this happening and not being critical.
The simplest case: Broadcom team releases a new firmware which
supports extra 5 GHz channels (e.g. due to the IEEE standard change).
Why should we refuse to run & support all "old" channel just because of that?
What do you mean by "make sense of what firmware provides"? Would kind
of solution would you suggest?
--
Rafał
