wifi: brcmfmac: Adjust n_channels usage for __counted_by

PATCH In review Kees Cook 6 participants 10 messages Jan 26, 2024 – Feb 2, 2024 Broadcom BRCM80211 IEEE802.11 wireless drivers

lore

Share & tools

lore

Kees Cook

author

2024-01-26 22:31 UTC #0 lore raw

PATCH 1/1 wifi: brcmfmac: Adjust n_channels usage for __counted_by

After commit e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by"), the compiler may enforce dynamic array indexing of req->channels to stay below n_channels. As a result, n_channels needs to be increased _before_ accessing the newly added array index. Increment it first, then use "i" for the prior index. Solves this warning in the coming GCC that has __counted_by support:

../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c: In function 'brcmf_internal_escan_add_info':
../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:3783:46: warning: operation on 'req-> n_channels' may be undefined [-Wsequence-point]

 3783 |                 req->channels[req->n_channels++] = chan;
      |                               ~~~~~~~~~~~~~~~^~

Fixes e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by")

Cc Arend van Spriel <[email protected]>

Cc Franky Lin <[email protected]>

Cc Hante Meuleman <[email protected]>

Cc Kalle Valo <[email protected]>

Cc Chi-hsien Lin <[email protected]>

Cc Ian Lin <[email protected]>

Cc Johannes Berg <[email protected]>

Cc Wright Feng <[email protected]>

Cc Hector Martin <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +4 -2

index 133c5ea6429c..28d6a30cc010 100644
@@ -3779,8 +3779,10 @@ static int brcmf_internal_escan_add_info(struct cfg80211_scan_request *req,
 		if (req->channels[i] == chan)
 			break;
 	}
-	if (i == req->n_channels)
-		req->channels[req->n_channels++] = chan;
+	if (i == req->n_channels) {
+		req->n_channels++;
+		req->channels[i] = chan;
+	}
 
 	for (i = 0; i < req->n_ssids; i++) {
 		if (req->ssids[i].ssid_len == ssid_len &&

Signature · 1 line

2.34.1

Hans de Goede

Reviewed-by on v1 1/1

2024-01-27 13:15 UTC #1 lore raw

Hi,

Quoted 13 lines · Kees Cook

After commit e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by"), the compiler may enforce dynamic array indexing of req->channels to stay below n_channels. As a result, n_channels needs to be increased _before_ accessing the newly added array index. Increment it first, then use "i" for the prior index. Solves this warning in the coming GCC that has __counted_by support:
../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c: In function 'brcmf_internal_escan_add_info':
../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:3783:46: warning: operation on 'req-> n_channels' may be undefined [-Wsequence-point]
 3783 |                 req->channels[req->n_channels++] = chan;
      |                               ~~~~~~~~~~~~~~~^~

Quoted 13 lines

Fixes e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by")
Cc Arend van Spriel <[email protected]>
Cc Franky Lin <[email protected]>
Cc Hante Meuleman <[email protected]>
Cc Kalle Valo <[email protected]>
Cc Chi-hsien Lin <[email protected]>
Cc Ian Lin <[email protected]>
Cc Johannes Berg <[email protected]>
Cc Wright Feng <[email protected]>
Cc Hector Martin <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>

Thanks, patch looks good to me:

Reviewed-by Hans de Goede <[email protected]>

Regards,

Hans

Quoted 21 lines

---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +4 -2

index 133c5ea6429c..28d6a30cc010 100644
@@ -3779,8 +3779,10 @@ static int brcmf_internal_escan_add_info(struct cfg80211_scan_request *req,
 		if (req->channels[i] == chan)
 			break;
 	}
-	if (i == req->n_channels)
-		req->channels[req->n_channels++] = chan;
+	if (i == req->n_channels) {
+		req->n_channels++;
+		req->channels[i] = chan;
+	}
 
 	for (i = 0; i < req->n_ssids; i++) {
 		if (req->ssids[i].ssid_len == ssid_len &&

Linus Walleij

Reviewed-by on v1 1/1

2024-01-27 16:37 UTC #2 lore raw

In reply to Kees Cook

Quoted 6 lines · Kees Cook

After commit e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by"), the compiler may enforce dynamic array indexing of req->channels to stay below n_channels. As a result, n_channels needs to be increased _before_ accessing the newly added array index. Increment it first, then use "i" for the prior index. Solves this warning in the coming GCC that has __counted_by support:

Makes perfect sense.

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Gustavo A. R. Silva

Reviewed-by on v1 1/1

2024-01-27 17:00 UTC #3 lore raw

In reply to Kees Cook

Quoted 13 lines · Kees Cook

After commit e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by"), the compiler may enforce dynamic array indexing of req->channels to stay below n_channels. As a result, n_channels needs to be increased _before_ accessing the newly added array index. Increment it first, then use "i" for the prior index. Solves this warning in the coming GCC that has __counted_by support:
../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c: In function 'brcmf_internal_escan_add_info':
../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:3783:46: warning: operation on 'req-> n_channels' may be undefined [-Wsequence-point]
  3783 |                 req->channels[req->n_channels++] = chan;
       |                               ~~~~~~~~~~~~~~~^~

Quoted 13 lines

Fixes e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by")
Cc Arend van Spriel <[email protected]>
Cc Franky Lin <[email protected]>
Cc Hante Meuleman <[email protected]>
Cc Kalle Valo <[email protected]>
Cc Chi-hsien Lin <[email protected]>
Cc Ian Lin <[email protected]>
Cc Johannes Berg <[email protected]>
Cc Wright Feng <[email protected]>
Cc Hector Martin <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>

Reviewed-by Gustavo A. R. Silva <[email protected]>

Thanks!

Signature · 1 line

Gustavo

Quoted 21 lines

---
  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +4 -2

index 133c5ea6429c..28d6a30cc010 100644
@@ -3779,8 +3779,10 @@ static int brcmf_internal_escan_add_info(struct cfg80211_scan_request *req,
  		if (req->channels[i] == chan)
  			break;
  	}
-	if (i == req->n_channels)
-		req->channels[req->n_channels++] = chan;
+	if (i == req->n_channels) {
+		req->n_channels++;
+		req->channels[i] = chan;
+	}
 
  	for (i = 0; i < req->n_ssids; i++) {
  		if (req->ssids[i].ssid_len == ssid_len &&

Kalle Valo

2024-02-01 10:04 UTC #4 lore raw

In reply to Kees Cook

Quoted 13 lines · Kees Cook

After commit e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by"), the compiler may enforce dynamic array indexing of req->channels to stay below n_channels. As a result, n_channels needs to be increased _before_ accessing the newly added array index. Increment it first, then use "i" for the prior index. Solves this warning in the coming GCC that has __counted_by support:
../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c: In function 'brcmf_internal_escan_add_info':
../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:3783:46: warning: operation on 'req-> n_channels' may be undefined [-Wsequence-point]
 3783 |                 req->channels[req->n_channels++] = chan;
      |                               ~~~~~~~~~~~~~~~^~

Quoted 16 lines

Fixes e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by")
Cc Arend van Spriel <[email protected]>
Cc Franky Lin <[email protected]>
Cc Hante Meuleman <[email protected]>
Cc Kalle Valo <[email protected]>
Cc Chi-hsien Lin <[email protected]>
Cc Ian Lin <[email protected]>
Cc Johannes Berg <[email protected]>
Cc Wright Feng <[email protected]>
Cc Hector Martin <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
Reviewed-by Hans de Goede <[email protected]>
Reviewed-by Linus Walleij <[email protected]>
Reviewed-by Gustavo A. R. Silva <[email protected]>

I'm planning to queue this for wireless tree. Arend, ack?

Signature · 3 lines

https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/

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

Arend Van Spriel

2024-02-02 09:58 UTC #5 lore raw

Quoted 32 lines · Kalle Valo

Quoted 13 lines · Kees Cook
After commit e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by"), the compiler may enforce dynamic array indexing of req->channels to stay below n_channels. As a result, n_channels needs to be increased _before_ accessing the newly added array index. Increment it first, then use "i" for the prior index. Solves this warning in the coming GCC that has __counted_by support:
../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c: In function 'brcmf_internal_escan_add_info':
../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:3783:46: warning: operation on 'req-> n_channels' may be undefined [-Wsequence-point]
  3783 |                 req->channels[req->n_channels++] = chan;
       |                               ~~~~~~~~~~~~~~~^~
Quoted 16 lines
Fixes e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by")
Cc Arend van Spriel <[email protected]>
Cc Franky Lin <[email protected]>
Cc Hante Meuleman <[email protected]>
Cc Kalle Valo <[email protected]>
Cc Chi-hsien Lin <[email protected]>
Cc Ian Lin <[email protected]>
Cc Johannes Berg <[email protected]>
Cc Wright Feng <[email protected]>
Cc Hector Martin <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
Reviewed-by Hans de Goede <[email protected]>
Reviewed-by Linus Walleij <[email protected]>
Reviewed-by Gustavo A. R. Silva <[email protected]>

Quoted 1 line

I'm planning to queue this for wireless tree. Arend, ack?

This slipped past my broadcom email. As the Fixes commit is in 6.7 I would say ACK.

Gr. AvS

Arend Van Spriel

2024-02-02 10:00 UTC #6 lore raw

Quoted 33 lines · Arend Van Spriel

Quoted 32 lines · Kalle Valo
Quoted 29 lines · Kees Cook
After commit e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by"), the compiler may enforce dynamic array indexing of req->channels to stay below n_channels. As a result, n_channels needs to be increased _before_ accessing the newly added array index. Increment it first, then use "i" for the prior index. Solves this warning in the coming GCC that has __counted_by support:
../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c: In function 'brcmf_internal_escan_add_info':
../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:3783:46: warning: operation on 'req-> n_channels' may be undefined [-Wsequence-point]
3783 | req->channels[req->n_channels++] = chan;
| ~~~~~~~~~~~~~~~^~
Fixes e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by")
Cc Arend van Spriel <[email protected]>
Cc Franky Lin <[email protected]>
Cc Hante Meuleman <[email protected]>
Cc Kalle Valo <[email protected]>
Cc Chi-hsien Lin <[email protected]>
Cc Ian Lin <[email protected]>
Cc Johannes Berg <[email protected]>
Cc Wright Feng <[email protected]>
Cc Hector Martin <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
Reviewed-by Hans de Goede <[email protected]>
Reviewed-by Linus Walleij <[email protected]>
Reviewed-by Gustavo A. R. Silva <[email protected]>

Quoted 3 lines

Quoted 1 line
I'm planning to queue this for wireless tree. Arend, ack?
This slipped past my broadcom email. As the Fixes commit is in 6.7 I would say ACK.

Cc to stable?

Gr. AvS

Kalle Valo

2024-02-02 10:35 UTC #7 lore raw

Quoted 37 lines · Arend Van Spriel

Quoted 35 lines · Arend Van Spriel
Quoted 34 lines · Kalle Valo
Quoted 31 lines · Kees Cook
After commit e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by"), the compiler may enforce dynamic array indexing of req->channels to stay below n_channels. As a result, n_channels needs to be increased _before_ accessing the newly added array index. Increment it first, then use "i" for the prior index. Solves this warning in the coming GCC that has __counted_by support:
../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c: In
function 'brcmf_internal_escan_add_info':
../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:3783:46: warning: operation on 'req-> n_channels' may be undefined [-Wsequence-point]
3783 | req->channels[req->n_channels++] = chan;
| ~~~~~~~~~~~~~~~^~
Fixes e3eac9f32ec0 ("wifi: cfg80211: Annotate struct
cfg80211_scan_request with __counted_by")
Cc Arend van Spriel <[email protected]>
Cc Franky Lin <[email protected]>
Cc Hante Meuleman <[email protected]>
Cc Kalle Valo <[email protected]>
Cc Chi-hsien Lin <[email protected]>
Cc Ian Lin <[email protected]>
Cc Johannes Berg <[email protected]>
Cc Wright Feng <[email protected]>
Cc Hector Martin <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
Reviewed-by Hans de Goede <[email protected]>
Reviewed-by Linus Walleij <[email protected]>
Reviewed-by Gustavo A. R. Silva <[email protected]>

Quoted 3 lines

Quoted 3 lines
Quoted 1 line
I'm planning to queue this for wireless tree. Arend, ack?
This slipped past my broadcom email. As the Fixes commit is in 6.7 I would say ACK.

Thanks.

Quoted 1 line

Cc to stable?

Is commit e3eac9f32ec0 in stable releases? (I don't follow stable and don't know what commits they take.) I propose that as we have Fixes tag let's not add cc but instead let stable maintainers to decide.

Signature · 3 lines

https://patchwork.kernel.org/project/linux-wireless/list/

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

Arend Van Spriel

2024-02-02 18:08 UTC #8 lore raw

Quoted 43 lines · Kalle Valo

Quoted 37 lines · Arend Van Spriel
Quoted 35 lines · Arend Van Spriel
Quoted 34 lines · Kalle Valo
Quoted 31 lines · Kees Cook
After commit e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by"), the compiler may enforce dynamic array indexing of req->channels to stay below n_channels. As a result, n_channels needs to be increased _before_ accessing the newly added array index. Increment it first, then use "i" for the prior index. Solves this warning in the coming GCC that has __counted_by support:
../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c: In
function 'brcmf_internal_escan_add_info':
../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:3783:46: warning: operation on 'req-> n_channels' may be undefined [-Wsequence-point]
3783 | req->channels[req->n_channels++] = chan;
| ~~~~~~~~~~~~~~~^~
Fixes e3eac9f32ec0 ("wifi: cfg80211: Annotate struct
cfg80211_scan_request with __counted_by")
Cc Arend van Spriel <[email protected]>
Cc Franky Lin <[email protected]>
Cc Hante Meuleman <[email protected]>
Cc Kalle Valo <[email protected]>
Cc Chi-hsien Lin <[email protected]>
Cc Ian Lin <[email protected]>
Cc Johannes Berg <[email protected]>
Cc Wright Feng <[email protected]>
Cc Hector Martin <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
Reviewed-by Hans de Goede <[email protected]>
Reviewed-by Linus Walleij <[email protected]>
Reviewed-by Gustavo A. R. Silva <[email protected]>
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
I'm planning to queue this for wireless tree. Arend, ack?
This slipped past my broadcom email. As the Fixes commit is in 6.7 I would say ACK.

Quoted 7 lines

Thanks.
Quoted 1 line
Cc to stable?
Is commit e3eac9f32ec0 in stable releases? (I don't follow stable and don't know what commits they take.) I propose that as we have Fixes tag let's not add cc but instead let stable maintainers to decide.

I confirmed the commit was in 6.7 and the latest released kernel is always handled by stable kernel team. kernel.org main page always shows the active stable/longterm releases. That said I have no problem with your proposal.

Gr. AvS

Kalle Valo

2024-02-02 16:45 UTC #9 lore raw

In reply to Kees Cook

Quoted 13 lines · Kees Cook

After commit e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by"), the compiler may enforce dynamic array indexing of req->channels to stay below n_channels. As a result, n_channels needs to be increased _before_ accessing the newly added array index. Increment it first, then use "i" for the prior index. Solves this warning in the coming GCC that has __counted_by support:
../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c: In function 'brcmf_internal_escan_add_info':
../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:3783:46: warning: operation on 'req-> n_channels' may be undefined [-Wsequence-point]
 3783 |                 req->channels[req->n_channels++] = chan;
      |                               ~~~~~~~~~~~~~~~^~

Quoted 16 lines

Fixes e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by")
Cc Arend van Spriel <[email protected]>
Cc Franky Lin <[email protected]>
Cc Hante Meuleman <[email protected]>
Cc Kalle Valo <[email protected]>
Cc Chi-hsien Lin <[email protected]>
Cc Ian Lin <[email protected]>
Cc Johannes Berg <[email protected]>
Cc Wright Feng <[email protected]>
Cc Hector Martin <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
Reviewed-by Hans de Goede <[email protected]>
Reviewed-by Linus Walleij <[email protected]>
Reviewed-by Gustavo A. R. Silva <[email protected]>

Patch applied to wireless.git, thanks.

5bdda0048c8d wifi: brcmfmac: Adjust n_channels usage for __counted_by

Signature · 3 lines

https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/

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