[PATCH 4/4] wifi: ath12k: Fix uninitialized variable access in ath12k_mac_allocate() function

[Karthikeyan Periyasamy]

On 12/12/2024 1:26 PM, Kalle Valo wrote:
> Karthikeyan Periyasamy <quic_periyasa at quicinc.com> writes:
> 
>> Currently, the uninitialized variable 'ab' is accessed in the
>> ath12k_mac_allocate() function. Initialize 'ab' with the first radio device
>> present in the hardware abstraction handle (ah). Additionally, move the
>> default setting procedure from the pdev mapping iteration to the total
>> radio calculating iteration for better code readability. Perform the
>> maximum radio validation check for total_radio to ensure that both num_hw
>> and radio_per_hw are validated indirectly, as these variables are derived
>> from total_radio. This also fixes the below Smatch static checker warning.
>>
>> Smatch warning:
>> ath12k_mac_allocate() error: uninitialized symbol 'ab'
>>
>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
>>
>> Fixes: a343d97f27f5 ("wifi: ath12k: move struct ath12k_hw from per device to group")
>> Signed-off-by: Karthikeyan Periyasamy <quic_periyasa at quicinc.com>
>> ---
>>   drivers/net/wireless/ath/ath12k/mac.c | 27 +++++++++++++++++++++------
>>   1 file changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
>> index 5cdc1c38b049..98b2f853d243 100644
>> --- a/drivers/net/wireless/ath/ath12k/mac.c
>> +++ b/drivers/net/wireless/ath/ath12k/mac.c
>> @@ -10962,8 +10962,20 @@ int ath12k_mac_allocate(struct ath12k_hw_group *ag)
>>   	u8 radio_per_hw;
>>   
>>   	total_radio = 0;
>> -	for (i = 0; i < ag->num_devices; i++)
>> -		total_radio += ag->ab[i]->num_radios;
>> +	for (i = 0; i < ag->num_devices; i++) {
>> +		ab = ag->ab[i];
>> +		if (!ab)
>> +			continue;
>> +
>> +		ath12k_mac_set_device_defaults(ab);
>> +		total_radio += ab->num_radios;
>> +	}
>> +
>> +	if (!total_radio)
>> +		return -EINVAL;
> 
> 'total_radio == 0' is more readable as it's a counter. Also please add ath12k_warn()
> 

This condition came due to no device handle (ab) present in the 
ath12k_hw_group (ag). ath12k_warn() cannot be used since no device (ab) 
present here. Additionally, ath12k_hw_warn() also cannot be used here 
since ath12k_hw (ah) also not created.

In future, we can introduce device (ab) and hardware (ah) agnostic 
warning helper function ath12k*dbg() for this usecase.

>> +
>> +	if (WARN_ON(total_radio > ATH12K_GROUP_MAX_RADIO))
>> +		return -ENOSPC;
> 
> BTW ath12k_warn() is preferred over WARN_ON(), but this is just for the
> future as this WARN_ON() was already there before.
> 

Sure

>>   
>>   	/* All pdev get combined and register as single wiphy based on
>>   	 * hardware group which participate in multi-link operation else
>> @@ -10976,14 +10988,16 @@ int ath12k_mac_allocate(struct ath12k_hw_group *ag)
>>   
>>   	num_hw = total_radio / radio_per_hw;
>>   
>> -	if (WARN_ON(num_hw >= ATH12K_GROUP_MAX_RADIO))
>> -		return -ENOSPC;
>> -
>>   	ag->num_hw = 0;
>>   	device_id = 0;
>>   	mac_id = 0;
>>   	for (i = 0; i < num_hw; i++) {
>>   		for (j = 0; j < radio_per_hw; j++) {
>> +			if (device_id >= ag->num_devices || !ag->ab[device_id]) {
>> +				ret = -ENOSPC;
>> +				goto err;
>> +			}
> 
> ath12k_warn()
> 

same here.

-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி