| Message ID | 20170103083858.6981-1-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 3-1-2017 9:38, 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. > > 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@milecki.pl> > --- > 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. 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. > --- > .../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. > - 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? > + 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. Regards, Arend
On 3 January 2017 at 12:02, Arend Van Spriel <arend.vanspriel@broadcom.com> wrote: > On 3-1-2017 9:38, 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. >> >> 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@milecki.pl> >> --- >> 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?
On 3-1-2017 12:31, Rafał Miłecki wrote: >>> + 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? 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. Regards, Arend
On 3 January 2017 at 14:19, Arend Van Spriel <arend.vanspriel@broadcom.com> wrote: > On 3-1-2017 12:31, Rafał Miłecki wrote: >>>> + 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? > > 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.
On 3 January 2017 at 15:14, Rafał Miłecki <zajec5@gmail.com> wrote: > On 3 January 2017 at 14:19, Arend Van Spriel > <arend.vanspriel@broadcom.com> wrote: >> On 3-1-2017 12:31, Rafał Miłecki wrote: >>>>> + 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? >> >> 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.
Arend Van Spriel <arend.vanspriel@broadcom.com> writes: > On 3-1-2017 9:38, 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. >> >> 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@milecki.pl> >> --- >> 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. > > 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. That's also my preference. And I read somewhere (forgot where) that in kernel summit there was a discussion about having only regression fixes in -rc kernels. So the rules are getting stricter, which is a good thing as then we can make releases in a shorter cycle.
Rafał Miłecki <zajec5@gmail.com> writes: > On 3 January 2017 at 15:14, Rafał Miłecki <zajec5@gmail.com> wrote: >> On 3 January 2017 at 14:19, Arend Van Spriel >> <arend.vanspriel@broadcom.com> wrote: >>> On 3-1-2017 12:31, Rafał Miłecki wrote: >>>>>> + 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? >>> >>> 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.
Rafał Miłecki <zajec5@gmail.com> writes: >>> @@ -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. If you are making a patch to stable or -rc releases you should keep the patch as simple as possible and do all the cleanup later. But I see that you dropped "cc stable" in this patch so all is good, just a general remark.
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; } } - channel[index].center_freq = - ieee80211_channel_to_frequency(ch.control_ch_num, - band->band); - channel[index].hw_value = ch.control_ch_num; + if (!channel) { + brcmf_err("Firmware reported unexpected channel %d\n", + ch.control_ch_num); + continue; + } /* assuming the chanspecs order is HT20, * HT40 upper, HT40 lower, and VHT80. */ if (ch.bw == BRCMU_CHAN_BW_80) { - channel[index].flags &= ~IEEE80211_CHAN_NO_80MHZ; + channel->flags &= ~IEEE80211_CHAN_NO_80MHZ; } else if (ch.bw == BRCMU_CHAN_BW_40) { - brcmf_update_bw40_channel_flag(&channel[index], &ch); + brcmf_update_bw40_channel_flag(channel, &ch); } else { /* enable the channel and disable other bandwidths * for now as mentioned order assure they are enabled * for subsequent chanspecs. */ - channel[index].flags = IEEE80211_CHAN_NO_HT40 | - IEEE80211_CHAN_NO_80MHZ; + channel->flags = IEEE80211_CHAN_NO_HT40 | + IEEE80211_CHAN_NO_80MHZ; ch.bw = BRCMU_CHAN_BW_20; cfg->d11inf.encchspec(&ch); chaninfo = ch.chspec; @@ -5907,11 +5906,11 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg, &chaninfo); if (!err) { if (chaninfo & WL_CHAN_RADAR) - channel[index].flags |= + channel->flags |= (IEEE80211_CHAN_RADAR | IEEE80211_CHAN_NO_IR); if (chaninfo & WL_CHAN_PASSIVE) - channel[index].flags |= + channel->flags |= IEEE80211_CHAN_NO_IR; } }