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

[Kalle Valo <kvalo@kernel.org>]

Karthikeyan Periyasamy <quic_periyasa@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@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()

> +
> +	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.

>  
>  	/* 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()

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

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