When compiling with gcc 13.1 and CONFIG_FORTIFY_SOURCE=y,
I've noticed the following:
In function ‘fortify_memcpy_chk’,
inlined from ‘ath_tx_complete_aggr’ at drivers/net/wireless/ath/ath9k/xmit.c:556:4,
inlined from ‘ath_tx_process_buffer’ at drivers/net/wireless/ath/ath9k/xmit.c:773:3:
./include/linux/fortify-string.h:529:25: warning: call to ‘__read_overflow2_field’
declared with attribute warning: detected read beyond size of field (2nd parameter);
maybe use struct_group()? [-Wattribute-warning]
529 | __read_overflow2_field(q_size_field, size);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function ‘fortify_memcpy_chk’,
inlined from ‘ath_tx_count_frames’ at drivers/net/wireless/ath/ath9k/xmit.c:473:3,
inlined from ‘ath_tx_complete_aggr’ at drivers/net/wireless/ath/ath9k/xmit.c:572:2,
inlined from ‘ath_tx_process_buffer’ at drivers/net/wireless/ath/ath9k/xmit.c:773:3:
./include/linux/fortify-string.h:529:25: warning: call to ‘__read_overflow2_field’
declared with attribute warning: detected read beyond size of field (2nd parameter);
maybe use struct_group()? [-Wattribute-warning]
529 | __read_overflow2_field(q_size_field, size);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In both cases, the compiler complains on:
memcpy(ba, &ts->ba_low, WME_BA_BMP_SIZE >> 3);
which is the legal way to copy both 'ba_low' and following 'ba_high'
members of 'struct ath_tx_status' at once (that is, issue one 8-byte
'memcpy()' for two 4-byte fields). Since the fortification logic
seems interprets this trick as an attempt to overread 4-byte 'ba_low',
relevant warnings may be silenced by specifying source buffer with
'offsetof()' of the 'ba_low' instead of using an address of the latter.
Signed-off-by: Dmitry Antipov <[email protected]>
---
drivers/net/wireless/ath/ath9k/xmit.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 8babaaacacf5..5a70395a3a2a 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -470,7 +470,8 @@ static void ath_tx_count_frames(struct ath_softc *sc, struct ath_buf *bf,
if (isaggr) {
seq_st = ts->ts_seqnum;
- memcpy(ba, &ts->ba_low, WME_BA_BMP_SIZE >> 3);
+ memcpy(ba, ts + offsetof(struct ath_tx_status, ba_low),
+ WME_BA_BMP_SIZE >> 3);
}
while (bf) {
@@ -553,7 +554,8 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
if (isaggr && txok) {
if (ts->ts_flags & ATH9K_TX_BA) {
seq_st = ts->ts_seqnum;
- memcpy(ba, &ts->ba_low, WME_BA_BMP_SIZE >> 3);
+ memcpy(ba, ts + offsetof(struct ath_tx_status, ba_low),
+ WME_BA_BMP_SIZE >> 3);
} else {
/*
* AR5416 can become deaf/mute when BA
--
2.41.0
On Tue, 2023-06-20 at 10:31 +0300, Dmitry Antipov wrote:
> When compiling with gcc 13.1 and CONFIG_FORTIFY_SOURCE=y,
> I've noticed the following:
>
> In function ‘fortify_memcpy_chk’,
> inlined from ‘ath_tx_complete_aggr’ at drivers/net/wireless/ath/ath9k/xmit.c:556:4,
> inlined from ‘ath_tx_process_buffer’ at drivers/net/wireless/ath/ath9k/xmit.c:773:3:
> ./include/linux/fortify-string.h:529:25: warning: call to ‘__read_overflow2_field’
> declared with attribute warning: detected read beyond size of field (2nd parameter);
> maybe use struct_group()? [-Wattribute-warning]
> 529 | __read_overflow2_field(q_size_field, size);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> In function ‘fortify_memcpy_chk’,
> inlined from ‘ath_tx_count_frames’ at drivers/net/wireless/ath/ath9k/xmit.c:473:3,
> inlined from ‘ath_tx_complete_aggr’ at drivers/net/wireless/ath/ath9k/xmit.c:572:2,
> inlined from ‘ath_tx_process_buffer’ at drivers/net/wireless/ath/ath9k/xmit.c:773:3:
> ./include/linux/fortify-string.h:529:25: warning: call to ‘__read_overflow2_field’
> declared with attribute warning: detected read beyond size of field (2nd parameter);
> maybe use struct_group()? [-Wattribute-warning]
> 529 | __read_overflow2_field(q_size_field, size);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> In both cases, the compiler complains on:
>
> memcpy(ba, &ts->ba_low, WME_BA_BMP_SIZE >> 3);
>
> which is the legal way to copy both 'ba_low' and following 'ba_high'
> members of 'struct ath_tx_status' at once (that is, issue one 8-byte
> 'memcpy()' for two 4-byte fields). Since the fortification logic
> seems interprets this trick as an attempt to overread 4-byte 'ba_low',
> relevant warnings may be silenced by specifying source buffer with
> 'offsetof()' of the 'ba_low' instead of using an address of the latter.
>
I think other places have typically fixed that with a struct_group, say
e.g. commit bfcc8ba45eb8 ("wifi: ath: Silence memcpy run-time false
positive warning"). To me, that seems slightly better since it still
ensures you're actually in the fields you wanted? But dunno.
johannes