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

[Kalle Valo <kvalo@codeaurora.org>]

Rafa=C5=82 Mi=C5=82ecki <zajec5@gmail.com> writes:

> On 3 January 2017 at 15:14, Rafa=C5=82 Mi=C5=82ecki <zajec5@gmail.com> wr=
ote:
>> On 3 January 2017 at 14:19, Arend Van Spriel
>> <arend.vanspriel@broadcom.com> wrote:
>>> On 3-1-2017 12:31, Rafa=C5=82 Mi=C5=82ecki wrote:
>>>>>> +             if (!channel) {
>>>>>> +                     brcmf_err("Firmware reported unexpected channe=
l %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 fa=
il.
>>>> Well, I could image something like this happening and not being critic=
al.
>>>> 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 o=
f that?
>>>
>>> Fair enough. I was assuming we keep __wl_{2,5}ghz_channels up to date
>>> with IEEE standard.
>>>
>>>> What do you mean by "make sense of what firmware provides"? Would kind
>>>> of solution would you suggest?
>>>
>>> When the above assumption can be assured (by us) the only other scenario
>>> would be a change in the firmware API where we wrongly interpret the
>>> information retrieved. In this case all subsequent channels will likely
>>> result in bogus or accidental matches hence it seems better to bail out
>>> early.
>>
>> Good point, this actually gave me an idea for a small nice
>> improvement. I remember I saw something like WL_VER in wl ioctl
>> protocol, it was already bumped at some point by Broadcom, when
>> chanspec format has changed. We should probably read this number from
>> firmware and maybe refuse to run if version is newer than the one we
>> know.
>
> I was thinking about WLC_GET_VERSION and you seem to already have it
> in brcmfmac under nam BRCMF_C_GET_VERSION. If you wish to be prepared
> for firmware API change, I guess you should check version. It seems
> brcmfmac supports 1 and 2.
>
> On the other hand if adding firmware with incompatible API you may
> want to have different directory or file names. I think this is what
> Intel does. This allows one to have multiple firmwares in
> /lib/firmware/ and switching between kernels freely.

ath10k does something similar. IIRC we currently support four different,
and incompatible, firmware releases now.

--=20
Kalle Valo