Re: [PATCH v2 1/8] ath10k: enhance swba event handler to adapt different size tim bitmap

[Kalle Valo <kvalo@xxxxxxxxxxxxxxxx>]

Kalle Valo <kvalo@xxxxxxxxxxxxxxxx> writes:

> Kalle Valo <kvalo@xxxxxxxxxxxxxxxx> writes:
>
>>>  	/* if next SWBA has no tim_changed the tim_bitmap is garbage.
>>>  	 * we must copy the bitmap upon change and reuse it later */
>>>  	if (__le32_to_cpu(tim_info->tim_changed)) {
>>>  		int i;
>>>  
>>> -		BUILD_BUG_ON(sizeof(arvif->u.ap.tim_bitmap) !=
>>> -			     sizeof(tim_info->tim_bitmap));
>>> +		WARN_ON(sizeof(arvif->u.ap.tim_bitmap) < tim_len);
>>
>> I'm worried that this WARN_ON() spams too much so I changed it to:
>>
>> --- a/drivers/net/wireless/ath/ath10k/wmi.c
>> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
>> @@ -2893,7 +2893,7 @@ static void ath10k_wmi_update_tim(struct ath10k *ar,
>>         if (__le32_to_cpu(tim_info->tim_changed)) {
>>                 int i;
>>  
>> -               WARN_ON(sizeof(arvif->u.ap.tim_bitmap) < tim_len);
>> +               WARN_ON_ONCE(sizeof(arvif->u.ap.tim_bitmap) < tim_len);
>>  
>>                 for (i = 0; i < tim_len; i++) {
>>                         t = tim_info->tim_bitmap[i / 4];
>
> Actually I got more worried about this. If tim_len >
> sizeof(arvif->u.ap.tim_bitmap) don't we read out of bounds? So we should
> actually add return for that case or am I missing something?
>
> Full code:
>
> 		WARN_ON_ONCE(sizeof(arvif->u.ap.tim_bitmap) < tim_len);
>
> 		for (i = 0; i < tim_len; i++) {
> 			t = tim_info->tim_bitmap[i / 4];
> 			v = __le32_to_cpu(t);
> 			arvif->u.ap.tim_bitmap[i] = (v >> ((i % 4) * 8)) & 0xFF;
> 		}

I talked with Raja and I changed this now to:

--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -2893,7 +2893,11 @@ static void ath10k_wmi_update_tim(struct ath10k *ar,
        if (__le32_to_cpu(tim_info->tim_changed)) {
                int i;
 
-               WARN_ON_ONCE(sizeof(arvif->u.ap.tim_bitmap) < tim_len);
+               if (sizeof(arvif->u.ap.tim_bitmap) < tim_len) {
+                       ath10k_warn(ar, "SWBA TIM field is too big (%d), truncated it to %d",
+                                   tim_len, sizeof(arvif->u.ap.tim_bitmap));
+                       tim_len = sizeof(arvif->u.ap.tim_bitmap);
+               }
 
                for (i = 0; i < tim_len; i++) {
                        t = tim_info->tim_bitmap[i / 4];

-- 
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html