carl9170: re-fix fortified-memset warning
PATCH 1/2 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.
--- drivers/net/wireless/ath/carl9170/tx.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/wireless/ath/carl9170/tx.c +2 -1
@@ -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
PATCH 2/2 mac80211: make ieee80211_tx_info padding explicit
From: Arnd Bergmann <[email protected]>
While looking at a bug, I got rather confused by the layout of the 'status' field in ieee80211_tx_info. Apparently, the intention is that status_driver_data[] is used for driver specific data, and fills up the size of the union to 40 bytes, just like the other ones.
This is indeed what actually happens, but only because of the combination of two mistakes:
- "void *status_driver_data[18 / sizeof(void *)];" is intended to be 18 bytes long but is actually two bytes shorter because of rounding-down in the division, to a multiple of the pointer size (4 bytes or 8 bytes).
- The other fields combined are intended to be 22 bytes long, but are actually 24 bytes because of padding in front of the unaligned tx_time member, and in front of the pointer array.
The two mistakes cancel out. so the size ends up fine, but it seems more helpful to make this explicit, by having a multiple of 8 bytes in the size calculation and explicitly describing the padding.
--- include/net/mac80211.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
include/net/mac80211.h +3 -1
@@ -1192,9 +1192,11 @@ struct ieee80211_tx_info {u8 ampdu_ack_len;u8 ampdu_len;u8 antenna; +u8 pad;u16 tx_time;u8 flags; -void *status_driver_data[18 /sizeof (void *)]; +u8 pad2; +void *status_driver_data[16 /sizeof (void *)]; } status;struct { struct ieee80211_tx_rate driver_rates[
Signature · 1 line
2.39.2
Re: [PATCH 2/2] mac80211: make ieee80211_tx_info padding explicit
Quoted 35 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
While looking at a bug, I got rather confused by the layout of the 'status' field in
ieee80211_tx_info. Apparently, the intention is that status_driver_data[] is used for driver specific data, and fills up the size of the union to 40 bytes, just like the other ones.This is indeed what actually happens, but only because of the combination of two mistakes:
- "void *status_driver_data[18 /
sizeof(void *)];" is intended to be 18 bytes long but is actually two bytes shorter because of rounding-down in the division, to a multiple of the pointer size (4 bytes or 8 bytes).- The other fields combined are intended to be 22 bytes long, but are actually 24 bytes because of padding in front of the unaligned
tx_timemember, and in front of the pointer array.The two mistakes cancel out. so the size ends up fine, but it seems more helpful to make this explicit, by having a multiple of 8 bytes in the size calculation and explicitly describing the padding.
Fixes ea5907db2a9cc ("mac80211: fix struct ieee80211_tx_info size")Fixes 02219b3abca59 ("mac80211: add WMM admission control support")Signed-off-by Arnd Bergmann <[email protected]>--- include/net/mac80211.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)include/net/mac80211.h +0
-0
include/net/mac80211.h struct ieee80211_tx_info { · line 1200
Quoted 10 lines
include/net/mac80211.h +3
-1@@ -1192,9 +1192,11 @@ struct ieee80211_tx_info {u8 ampdu_ack_len;u8 ampdu_len;u8 antenna; +u8 pad;u16 tx_time;u8 flags; -void *status_driver_data[18 /sizeof (void *)]; +u8 pad2; +void *status_driver_data[16 /sizeof (void *)]; } status;
pahole agrees with your assessment. :)
struct ieee80211_tx_info {
...
union {
...
struct {
struct ieee80211_tx_rate rates[4]; /* 8 12 */
s32 ack_signal; /* 20 4 */
u8 ampdu_ack_len; /* 24 1 */
u8 ampdu_len; /* 25 1 */
u8 antenna; /* 26 1 */
/* XXX 1 byte hole, try to pack */
u16 tx_time; /* 28 2 */
u8 flags; /* 30 1 */
/* XXX 1 byte hole, try to pack */
void * status_driver_data[2]; /* 32 16 */
} status; /* 8 40 */
struct {
struct ieee80211_tx_rate driver_rates[4]; /* 8 12 */
u8 pad[4]; /* 20 4 */
void * rate_driver_data[3]; /* 24 24 */
}; /* 8 40 */
void * driver_data[5]; /* 8 40 */
}; /* 8 40 */
...
};Signature · 1 line
Kees Cook
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 usestruct_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 thememset_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 7 lines
Unfortunately, the
memset_after()trick does not work on driver_rates[] because that is part of an anonymous struct, and I could not getstruct_group()to do this either. Using two separatememset()calls on the two members does address the warning though.Fixes fb5f6a0e8063b ("mac80211: Use memset_after() to clear tx status")Signed-off-by Arnd Bergmann <[email protected]>
Wait! I want to point out this funny thing is happening in ath too!
And that patch got NACK by Jiri Slaby because like me he suspects that this is a compiler bug.
so, what's going wrong with fortified there?
Thanks,
Christian
Quoted 8 lines · Christian Lamparter
On 6/23/23 17:23, Arnd Bergmann wrote:
Wait! I want to point out this funny thing is happening in ath too!
And that patch got NACK by Jiri Slaby because like me he suspects that this is a compiler bug.
FWIW, that is one I don't see with clang-17 or gcc-13. The one I'm addressing here is the only thing I see in ath wireless with the default set of warning options, though this driver does have a couple of others that are unrelated, when you enable the source data check in memcpy() by building with W=1.
In file included from drivers/net/wireless/ath/ath9k/xmit.c:17:
In file included from include/linux/dma-mapping.h:7:
In file included from include/linux/string.h:254:/home/arnd/arm-soc/include/linux/fortify-string.h:592:4: error: call to '__read_overflow2_field' declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
__read_overflow2_field(q_size_field, size);
^
include/linux/fortify-string.h:592:4: error: call to '__read_overflow2_field' declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
2 errors generated.
/home/arnd/arm-soc/include/linux/fortify-string.h:592:4: error: call to '__read_overflow2_field' declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
__read_overflow2_field(q_size_field, size);Quoted 1 line
so, what's going wrong with fortified there?
Kees might have a better answer to that, my best guess is that the one I'm addressing stems from the confusion between different union members.
Doing the randconfig builds with the latest compilers, carl9170 is the only one I see with fortified-string warnings, and there are a few dozen other drivers that I see with W=1, including one that affects all wireless drivers.
Arnd
Quoted 29 lines · Arnd Bergmann
Quoted 8 lines · Christian Lamparter
On 6/23/23 17:23, Arnd Bergmann wrote:
Wait! I want to point out this funny thing is happening in ath too!
And that patch got NACK by Jiri Slaby because like me he suspects that this is a compiler bug.
FWIW, that is one I don't see with clang-17 or gcc-13. The one I'm addressing here is the only thing I see in ath wireless with the default set of warning options, though this driver does have a couple of others that are unrelated, when you enable the source data check in
memcpy()by building with W=1.In file included from
drivers/net/wireless/ath/ath9k/xmit.c:17:
In file included frominclude/linux/dma-mapping.h:7:
In file included frominclude/linux/string.h:254:/home/arnd/arm-soc/include/linux/fortify-string.h:592:4: error: call to '__read_overflow2_field' declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe usestruct_group()? [-Werror,-Wattribute-warning]__read_overflow2_field(q_size_field, size); ^ include/linux/fortify-string.h:592:4: error: call to '__read_overflow2_field' declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror,-Wattribute-warning] 2 errors generated. /home/arnd/arm-soc/include/linux/fortify-string.h:592:4: error: call to '__read_overflow2_field' declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror,-Wattribute-warning] __read_overflow2_field(q_size_field, size);Quoted 1 line
so, what's going wrong with fortified there?
Quoted 8 lines
Kees might have a better answer to that, my best guess is that the one I'm addressing stems from the confusion between different union members.
Doing the randconfig builds with the latest compilers, carl9170 is the only one I see with fortified-string warnings, and there are a few dozen other drivers that I see with W=1, including one that affects all wireless drivers.
Hm, question here (to Jiri as well). Do you think that a workaround patch for these sort-of-obvious-but-compiler-bug-but-failed-to-make-a-simple-reproducer would be OK to get NACKed? In my case, I fiddled around with it and replaced the the cc_ani memset in the following way:
| memset(&common->cc_survey, 0, sizeof(common->cc_survey)); |- memset(&common->cc_ani, 0, sizeof(common->cc_ani)); |+ common->cc_ani.cycles = common->cc_ani.rx_busy = common->cc_ani.rx_frame = common->cc_ani.tx_frame = 0;
(Note here: cc_survey and cc_ani are of the same struct ath_cycle_counters! and they are right next to each other! Even better: reordering the memset in the code does not help. Reordering it in the ath_common does!)
This is less intrusive since it only replaces one line... but I'm afraid it too will get flagged for the same reason... Maybe can someone give Shiji Yang, or me, if he has lost interest, some guidance on how this can be addressed?
Best Regards,
Christian
Quoted 39 lines · Christian Lamparter
Quoted 29 lines · Arnd Bergmann
Quoted 8 lines · Christian Lamparter
On 6/23/23 17:23, Arnd Bergmann wrote:
Wait! I want to point out this funny thing is happening in ath too!
And that patch got NACK by Jiri Slaby because like me he suspects that this is a compiler bug.
FWIW, that is one I don't see with clang-17 or gcc-13. The one I'm addressing here is the only thing I see in ath wireless with the default set of warning options, though this driver does have a couple of others that are unrelated, when you enable the source data check in
memcpy()by building with W=1.In file included from
drivers/net/wireless/ath/ath9k/xmit.c:17:
In file included frominclude/linux/dma-mapping.h:7:
In file included frominclude/linux/string.h:254:/home/arnd/arm-soc/include/linux/fortify-string.h:592:4: error: call to '__read_overflow2_field' declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe usestruct_group()? [-Werror,-Wattribute-warning]__read_overflow2_field(q_size_field, size);
^include/linux/fortify-string.h:592:4: error: call to '__read_overflow2_field' declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe usestruct_group()? [-Werror,-Wattribute-warning] 2 errors generated./home/arnd/arm-soc/include/linux/fortify-string.h:592:4: error: call to '__read_overflow2_field' declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe usestruct_group()? [-Werror,-Wattribute-warning]__read_overflow2_field(q_size_field, size);Quoted 1 line
so, what's going wrong with fortified there?
Quoted 8 lines
Kees might have a better answer to that, my best guess is that the one I'm addressing stems from the confusion between different union members.
Doing the randconfig builds with the latest compilers, carl9170 is the only one I see with fortified-string warnings, and there are a few dozen other drivers that I see with W=1, including one that affects all wireless drivers.
Quoted 8 lines
Hm, question here (to Jiri as well). Do you think that a workaround patch for these sort-of-obvious-but-compiler-bug-but-failed-to-make-a-simple-reproducer would be OK to get NACKed? In my case, I fiddled around with it and replaced the the
cc_animemset in the following way:| memset(&common->cc_survey, 0, sizeof(common->cc_survey)); |- memset(&common->cc_ani, 0, sizeof(common->cc_ani)); |+ common->cc_ani.cycles = common->cc_ani.rx_busy = common->cc_ani.rx_frame = common->cc_ani.tx_frame = 0;
Nah, you are still changing the code for the compiler. And espectially this one calls for troubles later -- when cc_ani changes.
Again, work also with compiler guys, they are usually helpful. Both in helping to understand the issue (from the compiler POV) and provide a fix/workaround.
Even this carl9170 change looks very bad to me. While "memset_after(&txinfo->status, 0, rates);" means exactly what it does, those two memsets barely. It took me a while to understand what is going on and that it is the same. Don't do this.
Perhaps we need memset_no_check()?
thanks,
Signature · 2 lines
js suse labs
Quoted 4 lines · Arnd Bergmann
Doing the randconfig builds with the latest compilers, carl9170 is the only one I see with fortified-string warnings, and there are a few dozen other drivers that I see with W=1, including one that affects all wireless drivers.
Can you post the config that triggers this? I can't reproduce this warning...
-Kees
Signature · 1 line
Kees Cook
Quoted 35 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 usestruct_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 thememset_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 getstruct_group()to do this either. Using two separatememset()calls on the two members does address the warning though.Fixes fb5f6a0e8063b ("mac80211: Use memset_after() to clear tx status")Signed-off-by Arnd Bergmann <[email protected]>--- drivers/net/wireless/ath/carl9170/tx.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/net/wireless/ath/carl9170/tx.c +0
-0
drivers/net/wireless/ath/carl9170/tx.c static void carl9170_tx_release(struct kref *ref) · line 284
Quoted 6 lines
drivers/net/wireless/ath/carl9170/tx.c +2
-1@@ -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));
This is "accidentally" equivalent, which makes me nervous. It was designed to clear everything after "rates", regardless of padding, etc.
What I don't get is why the warning is being emitted. It boils down to an expansion of this:
memset(__ptr + offsetofend(typeof(*(obj)), member), __val,
sizeof(*(obj)) - offsetofend(typeof(*(obj)), member));into:
memset(&txinfo->status + offsetofend(struct ieee80211_tx_info, rates), 0, sizeof(txinfo->status - offsetofend(struct ieee80211_tx_info, rates)))
Is offsetofend() broken?
Signature · 1 line
Kees Cook