wifi: ath: false-positive fortified-memset warnings

PATCH In review Arnd Bergmann 5 participants 8 messages Mar 28, 2024 – Apr 4, 2024

Subsystems (3)

Qualcomm Atheros ATH9K wireless driver Atheros ath generic utilities CARL9170 Linux community wireless driver

lore

Share & tools

lore

Arnd Bergmann

author

2024-03-28 13:55 UTC #0 lore raw

cover wifi: ath: false-positive fortified-memset warnings

From: Arnd Bergmann <[email protected]>

While testing some other patch series I worked on across gcc versions, I found a couple of stringop warnings that only show up with some toolchains but not others. The warnings I both seem to be false positive.

I have also not found an explanation why both of these happen in atheros wireless drivers, as I don't see this in other drivers.

Maybe Kees can work out what is going on here.

Arnd Bergmann (2):
[RESEND] carl9170: re-fix fortified-memset warning
ath9k: work around memset overflow warning

 drivers/net/wireless/ath/ath.h         | 6 ++++--
 drivers/net/wireless/ath/ath9k/main.c  | 3 +--
 drivers/net/wireless/ath/carl9170/tx.c | 3 ++-
 3 files changed, 7 insertions(+), 5 deletions(-)

Signature · 1 line

2.39.2

Arnd Bergmann

author

2024-03-28 13:55 UTC #1 lore raw

PATCH RESEND 1/2 wifi: carl9170: re-fix fortified-memset warning

From: Arnd Bergmann <[email protected]>

The carl9170_tx_release() function sometimes triggers a fortified-memset warning in my randconfig builds:

In file included from include/linux/string.h:254,

                 from drivers/net/wireless/ath/carl9170/tx.c:40:
In function 'fortify_memset_chk',
    inlined from 'carl9170_tx_release' at drivers/net/wireless/ath/carl9170/tx.c:283:2,
    inlined from 'kref_put' at include/linux/kref.h:65:3,
    inlined from 'carl9170_tx_put_skb' at drivers/net/wireless/ath/carl9170/tx.c:342:9:

include/linux/fortify-string.h:493:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
493 | __write_overflow_field(p_size_field, size);

Kees previously tried to avoid this by using memset_after(), but it seems this does not fully address the problem. I noticed that the memset_after() here is done on a different part of the union (status) than the original cast was from (rate_driver_data), which may confuse the compiler.

Unfortunately, the memset_after() trick does not work on driver_rates[] because that is part of an anonymous struct, and I could not get struct_group() to do this either. Using two separate memset() calls on the two members does address the warning though.

Fixes fb5f6a0e8063b ("mac80211: Use memset_after() to clear tx status")

Link https://lore.kernel.org/lkml/[email protected]/ open here

Signed-off-by Arnd Bergmann <[email protected]>

---
I found this while testing randconfig builds, a .config that shows this for me is at https://pastebin.com/yWFKvZYu

Sorry I failed to follow up to Kees' request for a reproducer when I posted this last year.

---
 drivers/net/wireless/ath/carl9170/tx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/net/wireless/ath/carl9170/tx.c +2 -1

index e902ca80eba7..0226c31a6cae 100644
@@ -280,7 +280,8 @@ static void carl9170_tx_release(struct kref *ref)
 	 * carl9170_tx_fill_rateinfo() has filled the rate information
 	 * before we get to this point.
 	 */
-	memset_after(&txinfo->status, 0, rates);
+	memset(&txinfo->pad, 0, sizeof(txinfo->pad));
+	memset(&txinfo->rate_driver_data, 0, sizeof(txinfo->rate_driver_data));
 
 	if (atomic_read(&ar->tx_total_queued))
 		ar->tx_schedule = true;

Signature · 1 line

2.39.2

Kees Cook

Reviewed-by on v1 1/2

2024-03-28 21:51 UTC #2 lore raw

Re: [PATCH 1/2] [RESEND] wifi: carl9170: re-fix fortified-memset warning

Quoted 19 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>
The carl9170_tx_release() function sometimes triggers a fortified-memset warning in my randconfig builds:
In file included from include/linux/string.h:254,
                 from drivers/net/wireless/ath/carl9170/tx.c:40:
In function 'fortify_memset_chk',
    inlined from 'carl9170_tx_release' at drivers/net/wireless/ath/carl9170/tx.c:283:2,
    inlined from 'kref_put' at include/linux/kref.h:65:3,
    inlined from 'carl9170_tx_put_skb' at drivers/net/wireless/ath/carl9170/tx.c:342:9:
include/linux/fortify-string.h:493:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
493 | __write_overflow_field(p_size_field, size);
Kees previously tried to avoid this by using memset_after(), but it seems this does not fully address the problem. I noticed that the memset_after() here is done on a different part of the union (status) than the original cast was from (rate_driver_data), which may confuse the compiler.

Quoted 8 lines

Unfortunately, the memset_after() trick does not work on driver_rates[] because that is part of an anonymous struct, and I could not get struct_group() to do this either. Using two separate memset() calls on the two members does address the warning though.
Fixes fb5f6a0e8063b ("mac80211: Use memset_after() to clear tx status")
Link https://lore.kernel.org/lkml/[email protected]/ open here
Signed-off-by Arnd Bergmann <[email protected]>

Reviewed-by Kees Cook <[email protected]>

Signature · 1 line

Kees Cook

Christian Lamparter

Acked-by on v1 1/2

2024-03-29 12:56 UTC #3 lore raw

Re: [PATCH 1/2] [RESEND] wifi: carl9170: re-fix fortified-memset warning

In reply to Arnd Bergmann

Quoted 19 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>
The carl9170_tx_release() function sometimes triggers a fortified-memset warning in my randconfig builds:
In file included from include/linux/string.h:254,
                  from drivers/net/wireless/ath/carl9170/tx.c:40:
In function 'fortify_memset_chk',
     inlined from 'carl9170_tx_release' at drivers/net/wireless/ath/carl9170/tx.c:283:2,
     inlined from 'kref_put' at include/linux/kref.h:65:3,
     inlined from 'carl9170_tx_put_skb' at drivers/net/wireless/ath/carl9170/tx.c:342:9:
include/linux/fortify-string.h:493:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
493 | __write_overflow_field(p_size_field, size);
Kees previously tried to avoid this by using memset_after(), but it seems this does not fully address the problem. I noticed that the memset_after() here is done on a different part of the union (status) than the original cast was from (rate_driver_data), which may confuse the compiler.

Quoted 8 lines

Unfortunately, the memset_after() trick does not work on driver_rates[] because that is part of an anonymous struct, and I could not get struct_group() to do this either. Using two separate memset() calls on the two members does address the warning though.
Fixes fb5f6a0e8063b ("mac80211: Use memset_after() to clear tx status")
Link https://lore.kernel.org/lkml/[email protected]/ open here
Signed-off-by Arnd Bergmann <[email protected]>

Sure... though I think AI-supported compilers will in the future nag about this again. Oh well!

Acked-by Christian Lamparter <[email protected]>

Quoted 21 lines

---
I found this while testing randconfig builds, a .config that shows this for me is at https://pastebin.com/yWFKvZYu
Sorry I failed to follow up to Kees' request for a reproducer when I posted this last year.
---
  drivers/net/wireless/ath/carl9170/tx.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/wireless/ath/carl9170/tx.c +2 -1
index e902ca80eba7..0226c31a6cae 100644
@@ -280,7 +280,8 @@ static void carl9170_tx_release(struct kref *ref)
  	 * carl9170_tx_fill_rateinfo() has filled the rate information
  	 * before we get to this point.
  	 */
-	memset_after(&txinfo->status, 0, rates);
+	memset(&txinfo->pad, 0, sizeof(txinfo->pad));
+	memset(&txinfo->rate_driver_data, 0, sizeof(txinfo->rate_driver_data));

Kalle Valo

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

Re: [PATCH 1/2] [RESEND] wifi: carl9170: re-fix fortified-memset warning

In reply to Arnd Bergmann

Quoted 22 lines · Arnd Bergmann

The carl9170_tx_release() function sometimes triggers a fortified-memset warning in my randconfig builds:
In file included from include/linux/string.h:254,
                 from drivers/net/wireless/ath/carl9170/tx.c:40:
In function 'fortify_memset_chk',
    inlined from 'carl9170_tx_release' at drivers/net/wireless/ath/carl9170/tx.c:283:2,
    inlined from 'kref_put' at include/linux/kref.h:65:3,
    inlined from 'carl9170_tx_put_skb' at drivers/net/wireless/ath/carl9170/tx.c:342:9:
include/linux/fortify-string.h:493:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
493 | __write_overflow_field(p_size_field, size);
Kees previously tried to avoid this by using memset_after(), but it seems this does not fully address the problem. I noticed that the memset_after() here is done on a different part of the union (status) than the original cast was from (rate_driver_data), which may confuse the compiler.
Unfortunately, the memset_after() trick does not work on driver_rates[] because that is part of an anonymous struct, and I could not get struct_group() to do this either. Using two separate memset() calls on the two members does address the warning though.

Quoted 6 lines

Fixes fb5f6a0e8063b ("mac80211: Use memset_after() to clear tx status")
Link https://lore.kernel.org/lkml/[email protected]/ open here
Signed-off-by Arnd Bergmann <[email protected]>
Reviewed-by Kees Cook <[email protected]>
Acked-by Christian Lamparter <[email protected]>
Signed-off-by Kalle Valo <[email protected]>

2 patches applied to ath-next branch of ath.git, thanks.

066afafc10c9 wifi: carl9170: re-fix fortified-memset warning 61752ac69b69 wifi: ath9k: work around memset overflow warning

Signature · 3 lines

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

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

Arnd Bergmann

author

2024-03-28 13:55 UTC #5 lore raw

PATCH 2/2 wifi: ath9k: work around memset overflow warning

In reply to Arnd Bergmann

From: Arnd Bergmann <[email protected]>

gcc-9 and some other older versions produce a false-positive warning for zeroing two fields

In file included from include/linux/string.h:369,
from drivers/net/wireless/ath/ath9k/main.c:18:
In function 'fortify_memset_chk',
inlined from 'ath9k_ps_wakeup' at drivers/net/wireless/ath/ath9k/main.c:140:3:
include/linux/fortify-string.h:462:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]

  462 |                         __write_overflow_field(p_size_field, size);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Using a struct_group seems to reliably avoid the warning and not make the code much uglier. The combined memset() should even save a couple of cpu cycles.

Signed-off-by Arnd Bergmann <[email protected]>

---
This is from randconfig testing, see https://pastebin.com/yjKk5N81
for a reproducer

---
 drivers/net/wireless/ath/ath.h        | 6 ++++--
 drivers/net/wireless/ath/ath9k/main.c | 3 +--
 2 files changed, 5 insertions(+), 4 deletions(-)

drivers/net/wireless/ath/ath.h +4 -2

index f02a308a9ffc..34654f710d8a 100644
@@ -171,8 +171,10 @@ struct ath_common {
 	unsigned int clockrate;
 
 	spinlock_t cc_lock;
-	struct ath_cycle_counters cc_ani;
-	struct ath_cycle_counters cc_survey;
+	struct_group(cc,
+		struct ath_cycle_counters cc_ani;
+		struct ath_cycle_counters cc_survey;
+	);
 
 	struct ath_regulatory regulatory;
 	struct ath_regulatory reg_world_copy;

drivers/net/wireless/ath/ath9k/main.c +1 -2

index a2943aaecb20..01173aac3045 100644
@@ -135,8 +135,7 @@ void ath9k_ps_wakeup(struct ath_softc *sc)
 	if (power_mode != ATH9K_PM_AWAKE) {
 		spin_lock(&common->cc_lock);
 		ath_hw_cycle_counters_update(common);
-		memset(&common->cc_survey, 0, sizeof(common->cc_survey));
-		memset(&common->cc_ani, 0, sizeof(common->cc_ani));
+		memset(&common->cc, 0, sizeof(common->cc));
 		spin_unlock(&common->cc_lock);
 	}

Signature · 1 line

2.39.2

Toke Høiland-Jørgensen

maintainer Acked-by on v1 2/2

2024-03-28 21:14 UTC #6 lore raw

Re: [PATCH 2/2] wifi: ath9k: work around memset overflow warning

Quoted 10 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>
gcc-9 and some other older versions produce a false-positive warning for zeroing two fields
In file included from include/linux/string.h:369,
from drivers/net/wireless/ath/ath9k/main.c:18:
In function 'fortify_memset_chk',
inlined from 'ath9k_ps_wakeup' at drivers/net/wireless/ath/ath9k/main.c:140:3:
include/linux/fortify-string.h:462:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]

Quoted 8 lines

  462 |                         __write_overflow_field(p_size_field, size);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Using a struct_group seems to reliably avoid the warning and not make the code much uglier. The combined memset() should even save a couple of cpu cycles.
Signed-off-by Arnd Bergmann <[email protected]>

Acked-by Toke Høiland-Jørgensen <[email protected]>

Kees Cook

Reviewed-by on v1 2/2

2024-03-28 21:50 UTC #7 lore raw

Re: [PATCH 2/2] wifi: ath9k: work around memset overflow warning

In reply to Arnd Bergmann

Quoted 10 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>
gcc-9 and some other older versions produce a false-positive warning for zeroing two fields
In file included from include/linux/string.h:369,
from drivers/net/wireless/ath/ath9k/main.c:18:
In function 'fortify_memset_chk',
inlined from 'ath9k_ps_wakeup' at drivers/net/wireless/ath/ath9k/main.c:140:3:
include/linux/fortify-string.h:462:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]

Quoted 8 lines

  462 |                         __write_overflow_field(p_size_field, size);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Using a struct_group seems to reliably avoid the warning and not make the code much uglier. The combined memset() should even save a couple of cpu cycles.
Signed-off-by Arnd Bergmann <[email protected]>

Reviewed-by Kees Cook <[email protected]>

Signature · 1 line

Kees Cook