| Message ID | 20240704144341.207317-1-kvalo@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] wifi: ath12k: workaround fortify warnings in ath12k_wow_convert_8023_to_80211() | expand |
Kees Cook <kees@kernel.org> writes: > 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. Ah, thanks for correcting. I just saw fortify-string.h and made the wrong assumption. > 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). Earlier I did some testing and I noticed that this if test also gives a warning: 1 + eth_pat_len < ETH_ALEN But this doesn't have any warning: 0 + eth_pat_len < ETH_ALEN And I stopped my investigation there :) > Reviewed-by: Kees Cook <kees@kernel.org> So you think this should be applied? It's not really logical so I would prefer to avoid taking it if possible. Or should we just ignore the warning? It only happens on GCC 11 anyway.
On Mon, Jul 08, 2024 at 06:51:52PM +0300, Kalle Valo wrote: > Kees Cook <kees@kernel.org> writes: > > > 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. > > Ah, thanks for correcting. I just saw fortify-string.h and made the > wrong assumption. > > > 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). > > Earlier I did some testing and I noticed that this if test also gives a > warning: > > 1 + eth_pat_len < ETH_ALEN > > But this doesn't have any warning: > > 0 + eth_pat_len < ETH_ALEN > > And I stopped my investigation there :) > > > Reviewed-by: Kees Cook <kees@kernel.org> > > So you think this should be applied? It's not really logical so I would > prefer to avoid taking it if possible. Or should we just ignore the > warning? It only happens on GCC 11 anyway. I suspect this won't be the only place in the kernel where -Wrestrict will give weird results with GCC 11, and there are still plenty of folks using GCC 11. I think the best option would probably be to version-check GCC to gate the addition of -Wrestrict. Arnd, what do you think? This looks like a more extreme version of commit f9fc1ec28bae ("crypto: drivers - avoid memcpy size warning") -Kees
On Mon, Jul 8, 2024, at 21:31, Kees Cook wrote: > On Mon, Jul 08, 2024 at 06:51:52PM +0300, Kalle Valo wrote: > > I suspect this won't be the only place in the kernel where -Wrestrict > will give weird results with GCC 11, and there are still plenty of folks > using GCC 11. I think the best option would probably be to version-check > GCC to gate the addition of -Wrestrict. > > Arnd, what do you think? This looks like a more extreme version of > commit f9fc1ec28bae ("crypto: drivers - avoid memcpy size warning") The f9fc1ec28bae patch was the other way around, it showed up in new compilers but not old ones. I don't think I've seen more gcc-11 -Wrestrict warnings during testing, but I'm currently not set up to do a thorough search. If it's the only one, then Kalle's suggested workaround is probably best, but if there are additional warnings on gcc-11, making the warning depend newer compilers is also fine. I just don't want to give up the warning for new compilers altogether. Arnd
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. > > 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] > 114 | #define __underlying_memcpy __builtin_memcpy > | ^ > ./include/linux/fortify-string.h:637:9: note: in expansion of macro ‘__underlying_memcpy’ > 637 | __underlying_##op(p, q, __fortify_size); | ^~~~~~~~~~~~~ > ./include/linux/fortify-string.h:682:26: note: in expansion of macro ‘__fortify_memcpy_chk’ > 682 | #define memcpy(p, q, s) __fortify_memcpy_chk(p, q, s, | ^~~~~~~~~~~~~~~~~~~~ > drivers/net/wireless/ath/ath12k/wow.c:190:25: note: in expansion of macro ‘memcpy’ > 190 | memcpy(pat, eth_pat, eth_pat_len); > | ^~~~~~ > ./include/linux/fortify-string.h:114:33: error: ‘__builtin_memcpy’ accessing 18446744073709551605 or more bytes at offsets 0 and 0 overlaps 9223372036854775787 bytes at offset -9223372036854775798 [-Werror=restrict] > 114 | #define __underlying_memcpy __builtin_memcpy > | ^ > ./include/linux/fortify-string.h:637:9: note: in expansion of macro ‘__underlying_memcpy’ > 637 | __underlying_##op(p, q, __fortify_size); | ^~~~~~~~~~~~~ > ./include/linux/fortify-string.h:682:26: note: in expansion of macro ‘__fortify_memcpy_chk’ > 682 | #define memcpy(p, q, s) __fortify_memcpy_chk(p, q, s, | ^~~~~~~~~~~~~~~~~~~~ > drivers/net/wireless/ath/ath12k/wow.c:232:25: note: in expansion of macro ‘memcpy’ > 232 | memcpy(pat, eth_pat, eth_pat_len); > | ^~~~~~ > > Compile tested only. > > Reported-by: Johannes Berg <johannes@sipsolutions.net> > Fixes: 4a3c212eee0e ("wifi: ath12k: add basic WoW functionalities") > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> Reviewed-by: Paul E. McKenney <paulmck@kernel.org> (Also compile retested, for whatever that is worth.) This still shows up in allmodconfig next-20240709 builds. Thanx, Paul > --- > drivers/net/wireless/ath/ath12k/wow.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > 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); > > > base-commit: c1cacb01f35589bd41360cdb7535afc792c08a7c > -- > 2.39.2 >
"Arnd Bergmann" <arnd@arndb.de> writes: > On Mon, Jul 8, 2024, at 21:31, Kees Cook wrote: >> On Mon, Jul 08, 2024 at 06:51:52PM +0300, Kalle Valo wrote: >> >> I suspect this won't be the only place in the kernel where -Wrestrict >> will give weird results with GCC 11, and there are still plenty of folks >> using GCC 11. I think the best option would probably be to version-check >> GCC to gate the addition of -Wrestrict. >> >> Arnd, what do you think? This looks like a more extreme version of >> commit f9fc1ec28bae ("crypto: drivers - avoid memcpy size warning") > > The f9fc1ec28bae patch was the other way around, it showed up > in new compilers but not old ones. I don't think I've seen > more gcc-11 -Wrestrict warnings during testing, but I'm currently > not set up to do a thorough search. If it's the only one, then > Kalle's suggested workaround is probably best, but if there > are additional warnings on gcc-11, making the warning depend > newer compilers is also fine. Honestly I was hoping that we could disable the warning for GCC 11 :) I feel bad making the code worse due to a compiler problem. For example, Intel's zero day bot doesn't seem to use GCC 11 that much anymore, so it might surprise more people than just us ath12k folks. (The bot said everything was fine but Johannes saw the warning when the code was pulled to wireless-next.) > I just don't want to give up the warning for new compilers altogether. Me neither. I'm just hoping that we could disable it for GCC 11. But of course if you think it's better to add the workaround to ath12k, I can submit a proper (non-RFC) patch to do that.
Kalle Valo <kvalo@kernel.org> writes: > "Arnd Bergmann" <arnd@arndb.de> writes: > >> On Mon, Jul 8, 2024, at 21:31, Kees Cook wrote: >>> On Mon, Jul 08, 2024 at 06:51:52PM +0300, Kalle Valo wrote: >>> >>> I suspect this won't be the only place in the kernel where -Wrestrict >>> will give weird results with GCC 11, and there are still plenty of folks >>> using GCC 11. I think the best option would probably be to version-check >>> GCC to gate the addition of -Wrestrict. >>> >>> Arnd, what do you think? This looks like a more extreme version of >>> commit f9fc1ec28bae ("crypto: drivers - avoid memcpy size warning") >> >> The f9fc1ec28bae patch was the other way around, it showed up >> in new compilers but not old ones. I don't think I've seen >> more gcc-11 -Wrestrict warnings during testing, but I'm currently >> not set up to do a thorough search. If it's the only one, then >> Kalle's suggested workaround is probably best, but if there >> are additional warnings on gcc-11, making the warning depend >> newer compilers is also fine. > > Honestly I was hoping that we could disable the warning for GCC 11 :) > > I feel bad making the code worse due to a compiler problem. For example, > Intel's zero day bot doesn't seem to use GCC 11 that much anymore, so it > might surprise more people than just us ath12k folks. (The bot said > everything was fine but Johannes saw the warning when the code was > pulled to wireless-next.) > >> I just don't want to give up the warning for new compilers altogether. > > Me neither. I'm just hoping that we could disable it for GCC 11. But of > course if you think it's better to add the workaround to ath12k, I can > submit a proper (non-RFC) patch to do that. For the archives: Paolo found a nicer way which is now commited: https://git.kernel.org/linus/b49991d83bba
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);