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

[Kalle Valo <kvalo@qca.qualcomm.com>]

Kalle Valo <kvalo@qca.qualcomm.com> 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;
		}

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k