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

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

[Rafał Miłecki]

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.
---
 .../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;
 			}
 		}
-		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;
 			}
 		}
-- 
2.10.1

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

[Arend Van Spriel]

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

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

[Rafał Miłecki]

On 3 January 2017 at 12:02, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
> On 3-1-2017 9:38, Rafa=C5=82 Mi=C5=82ecki wrote:
>> From: Rafa=C5=82 Mi=C5=82ecki <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 wiph=
y bands")
>> Signed-off-by: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
>> ---
>> I'm not sure what kind of material it is. It fixes possible memory corru=
ption
>> (serious thing?) but this bug was there since Apr 2015, so is it worth f=
ixing
>> 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 w=
orking
>> 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_c=
fg80211_info *cfg,
>>       u32 i, j;
>>       u32 total;
>>       u32 chaninfo;
>> -     u32 index;
>>
>>       pbuf =3D kzalloc(BRCMF_DCMD_MEDLEN, GFP_KERNEL);
>>
>> @@ -5873,33 +5872,33 @@ static int brcmf_construct_chaninfo(struct brcmf=
_cfg80211_info *cfg,
>>                   ch.bw =3D=3D BRCMU_CHAN_BW_80)
>>                       continue;
>>
>> -             channel =3D band->channels;
>> -             index =3D band->n_channels;
>> +             channel =3D NULL;
>>               for (j =3D 0; j < band->n_channels; j++) {
>> -                     if (channel[j].hw_value =3D=3D ch.control_ch_num) =
{
>> -                             index =3D j;
>> +                     if (band->channels[j].hw_value =3D=3D ch.control_c=
h_num) {
>> +                             channel =3D &band->channels[j];
>>                               break;
>>                       }
>>               }
>
> You could have kept the index construct and simply check if j =3D=3D
> 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 =3D
>> -                     ieee80211_channel_to_frequency(ch.control_ch_num,
>> -                                                    band->band);
>> -             channel[index].hw_value =3D 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 tha=
t?

What do you mean by "make sense of what firmware provides"? Would kind
of solution would you suggest?

--=20
Rafa=C5=82

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

[Arend Van Spriel]

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

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

[Rafał Miłecki]

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 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.

--=20
Rafa=C5=82

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

[Rafał Miłecki]

On 3 January 2017 at 15:14, Rafa=C5=82 Mi=C5=82ecki <zajec5@gmail.com> wrot=
e:
> 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 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 fai=
l.
>>> Well, I could image something like this happening and not being critica=
l.
>>> 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.

--=20
Rafa=C5=82

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

[Rafał Miłecki]

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);
+			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 +5909,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;
 			}
 		}
-- 
2.10.1

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

[Kalle Valo]

Arend Van Spriel <arend.vanspriel@broadcom.com> writes:

> On 3-1-2017 9:38, Rafa=C5=82 Mi=C5=82ecki wrote:
>> From: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
>>=20
>> 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.
>>=20
>> Fix this by detecting unexpected channel and ignoring it.
>>=20
>> Fixes: 58de92d2f95e ("brcmfmac: use static superset of channels for wiph=
y bands")
>> Signed-off-by: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
>> ---
>> I'm not sure what kind of material it is. It fixes possible memory corru=
ption
>> (serious thing?) but this bug was there since Apr 2015, so is it worth f=
ixing
>> 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 w=
orking
>> 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.

--=20
Kalle Valo

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

[Kalle Valo]

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

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

[Kalle Valo]

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

>>> @@ -5873,33 +5872,33 @@ static int brcmf_construct_chaninfo(struct brcm=
f_cfg80211_info *cfg,
>>>                   ch.bw =3D=3D BRCMU_CHAN_BW_80)
>>>                       continue;
>>>
>>> -             channel =3D band->channels;
>>> -             index =3D band->n_channels;
>>> +             channel =3D NULL;
>>>               for (j =3D 0; j < band->n_channels; j++) {
>>> -                     if (channel[j].hw_value =3D=3D ch.control_ch_num)=
 {
>>> -                             index =3D j;
>>> +                     if (band->channels[j].hw_value =3D=3D ch.control_=
ch_num) {
>>> +                             channel =3D &band->channels[j];
>>>                               break;
>>>                       }
>>>               }
>>
>> You could have kept the index construct and simply check if j =3D=3D
>> 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.

--=20
Kalle Valo

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

[Arend Van Spriel]

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, ...).

Regards,
Arend

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

[Rafał Miłecki]

On 4 January 2017 at 10:39, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
> On 3-1-2017 17:49, Rafa=C5=82 Mi=C5=82ecki wrote:
>> From: Rafa=C5=82 Mi=C5=82ecki <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 wiph=
y bands")
>> Signed-off-by: Rafa=C5=82 Mi=C5=82ecki <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_c=
fg80211_info *cfg,
>>       u32 i, j;
>>       u32 total;
>>       u32 chaninfo;
>> -     u32 index;
>>
>>       pbuf =3D kzalloc(BRCMF_DCMD_MEDLEN, GFP_KERNEL);
>>
>> @@ -5873,33 +5872,36 @@ static int brcmf_construct_chaninfo(struct brcmf=
_cfg80211_info *cfg,
>>                   ch.bw =3D=3D BRCMU_CHAN_BW_80)
>>                       continue;
>>
>> -             channel =3D band->channels;
>> -             index =3D band->n_channels;
>> +             channel =3D NULL;
>>               for (j =3D 0; j < band->n_channels; j++) {
>> -                     if (channel[j].hw_value =3D=3D ch.control_ch_num) =
{
>> -                             index =3D j;
>> +                     if (band->channels[j].hw_value =3D=3D ch.control_c=
h_num) {
>> +                             channel =3D &band->channels[j];
>>                               break;
>>                       }
>>               }
>> -             channel[index].center_freq =3D
>> -                     ieee80211_channel_to_frequency(ch.control_ch_num,
>> -                                                    band->band);
>> -             channel[index].hw_value =3D ch.control_ch_num;
>> +             if (!channel) {
>> +                     /* It seems firmware supports some channel we neve=
r
>> +                      * 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.

--=20
Rafa=C5=82

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

[Arend Van Spriel]

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

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

[Rafał Miłecki]

On 4 January 2017 at 11:48, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
> On 4-1-2017 11:40, Rafa=C5=82 Mi=C5=82ecki wrote:
>> On 4 January 2017 at 10:39, Arend Van Spriel
>> <arend.vanspriel@broadcom.com> wrote:
>>> On 3-1-2017 17:49, Rafa=C5=82 Mi=C5=82ecki wrote:
>>>> From: Rafa=C5=82 Mi=C5=82ecki <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. Thi=
s
>>>> never happened so far (we got a complete list of supported channels) b=
ut
>>>> 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_val=
ue
>>>> and center_freq assignment. For known channels we have these set anywa=
y.
>>>>
>>>> I decided to fix this issue by assigning NULL or a target channel to t=
he
>>>> 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 wi=
phy bands")
>>>> Signed-off-by: Rafa=C5=82 Mi=C5=82ecki <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 =3D kzalloc(BRCMF_DCMD_MEDLEN, GFP_KERNEL);
>>>>
>>>> @@ -5873,33 +5872,36 @@ static int brcmf_construct_chaninfo(struct brc=
mf_cfg80211_info *cfg,
>>>>                   ch.bw =3D=3D BRCMU_CHAN_BW_80)
>>>>                       continue;
>>>>
>>>> -             channel =3D band->channels;
>>>> -             index =3D band->n_channels;
>>>> +             channel =3D NULL;
>>>>               for (j =3D 0; j < band->n_channels; j++) {
>>>> -                     if (channel[j].hw_value =3D=3D ch.control_ch_num=
) {
>>>> -                             index =3D j;
>>>> +                     if (band->channels[j].hw_value =3D=3D ch.control=
_ch_num) {
>>>> +                             channel =3D &band->channels[j];
>>>>                               break;
>>>>                       }
>>>>               }
>>>> -             channel[index].center_freq =3D
>>>> -                     ieee80211_channel_to_frequency(ch.control_ch_num=
,
>>>> -                                                    band->band);
>>>> -             channel[index].hw_value =3D ch.control_ch_num;
>>>> +             if (!channel) {
>>>> +                     /* It seems firmware supports some channel we ne=
ver
>>>> +                      * 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

... sorry, it seems I should take a break ;)

--=20
Rafa=C5=82

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

[Rafał Miłecki]

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
V3: Update error message as suggested by Arend.
---
 .../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..75ef23b 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("Ignoring unexpected firmware 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 +5909,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;
 			}
 		}
-- 
2.10.1

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

[Arend Van Spriel]

On 4-1-2017 12:09, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>

Not taking a break?

> 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")
Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> 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
> V3: Update error message as suggested by Arend.
> ---
>  .../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..75ef23b 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("Ignoring unexpected firmware 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 +5909,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;
>  			}
>  		}
> 

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

[Kalle Valo]

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>
> Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>

Patch applied to wireless-drivers-next.git, thanks.

77c0d0cd10e7 brcmfmac: avoid writing channel out of allocated array

-- 
https://patchwork.kernel.org/patch/9496471/

Documentation about submitting wireless patches and checking status
from patchwork:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches