Re: [PATCH RFC] wifi: ath12k: workaround fortify warnings in ath12k_wow_convert_8023_to_80211()

[Kalle Valo <kvalo@kernel.org>]

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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