| Message ID | 20240704144341.207317-1-kvalo@kernel.org (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [RFC] wifi: ath12k: workaround fortify warnings in ath12k_wow_convert_8023_to_80211() | expand |
On Thu, Jul 04, 2024 at 05:43:41PM +0300, Kalle Valo wrote: > From: Kalle Valo <quic_kvalo@quicinc.com> > > Johannes reported with GCC 11.4 there's a fortify warning below. The warning is > not seen with GCC 12.1 nor 13.2. Weirdly moving the other operand of sum to the > other side the warning goes away. This is safe to do as the value of the > operand is check earlier. But the code looks worse with this so I'm not sure > what to do. FWIW, this isn't fortify, but -Wrestrict. I would expect the same warnings even with CONFIG_FORTIFY_SOURCE disabled. Regardless, it's worth figuring out what's going on. It looks like this is GCC's value range tracker deciding it sees a way for things to go weird. I suspect they fixed -Wrestrict in later GCC versions. It might need to be version-limited... > In file included from ./include/linux/string.h:374, > from ./include/linux/bitmap.h:13, > from ./include/linux/cpumask.h:13, > from ./include/linux/sched.h:16, > from ./include/linux/delay.h:23, > from drivers/net/wireless/ath/ath12k/wow.c:7: > drivers/net/wireless/ath/ath12k/wow.c: In function ‘ath12k_wow_convert_8023_to_80211.constprop’: > ./include/linux/fortify-string.h:114:33: error: ‘__builtin_memcpy’ accessing 18446744073709551611 or more bytes at offsets 0 and 0 overlaps 9223372036854775799 bytes at offset -9223372036854775804 [-Werror=restrict] These huge negative values imply to me that GCC is looking at some signed values somewhere. > [...] > diff --git a/drivers/net/wireless/ath/ath12k/wow.c b/drivers/net/wireless/ath/ath12k/wow.c > index c5cba825a84a..e9588bb7561c 100644 > --- a/drivers/net/wireless/ath/ath12k/wow.c > +++ b/drivers/net/wireless/ath/ath12k/wow.c > @@ -186,7 +186,7 @@ ath12k_wow_convert_8023_to_80211(struct ath12k *ar, > if (eth_pkt_ofs < ETH_ALEN) { > pkt_ofs = eth_pkt_ofs + a1_ofs; > > - if (eth_pkt_ofs + eth_pat_len < ETH_ALEN) { > + if (eth_pat_len < ETH_ALEN - eth_pkt_ofs) { > memcpy(pat, eth_pat, eth_pat_len); > memcpy(bytemask, eth_bytemask, eth_pat_len); Both eth_pkt_ofs and eth_pat_len are size_t. ETH_ALEN isn't, but it would be promoted to size_t here. The value tracker should see that eth_pkt_ofs could be [0..ETH_ALEN). eth_pat_len is coming from an "int", though, so that might be the confusion. It may think eth_pat_len could be [0..UINT_MAX] (i.e. the full range of int within size_t). So [0..ETH_ALEN) + [0..UINT_MAX] < 6 might be doing something wrong in GCC 11.x, and it's not actually doing the size_t promotion correctly, or deciding something has wrapped and then thinking eth_pat_len could span a giant region of the address space, which freaks out -Wrestrict. i.e. it's seeing that for the "if" to be true, eth_pat_len could be large enough to wrap around the addition (though this shouldn't be possible for 64-bit size_t). So I could see how [0..UINT_MAX] < 6 - [0..ETH_ALEN) would make it happier: the right side is now [1..6], so eth_pat_len becomes [1..6). Reviewed-by: Kees Cook <kees@kernel.org>
diff --git a/drivers/net/wireless/ath/ath12k/wow.c b/drivers/net/wireless/ath/ath12k/wow.c index c5cba825a84a..e9588bb7561c 100644 --- a/drivers/net/wireless/ath/ath12k/wow.c +++ b/drivers/net/wireless/ath/ath12k/wow.c @@ -186,7 +186,7 @@ ath12k_wow_convert_8023_to_80211(struct ath12k *ar, if (eth_pkt_ofs < ETH_ALEN) { pkt_ofs = eth_pkt_ofs + a1_ofs; - if (eth_pkt_ofs + eth_pat_len < ETH_ALEN) { + if (eth_pat_len < ETH_ALEN - eth_pkt_ofs) { memcpy(pat, eth_pat, eth_pat_len); memcpy(bytemask, eth_bytemask, eth_pat_len); @@ -228,7 +228,7 @@ ath12k_wow_convert_8023_to_80211(struct ath12k *ar, } else if (eth_pkt_ofs < prot_ofs) { pkt_ofs = eth_pkt_ofs - ETH_ALEN + a3_ofs; - if (eth_pkt_ofs + eth_pat_len < prot_ofs) { + if (eth_pat_len < prot_ofs - eth_pkt_ofs) { memcpy(pat, eth_pat, eth_pat_len); memcpy(bytemask, eth_bytemask, eth_pat_len);