Re:Re: [PATCH] wifi: ath12k: validate MAC/PHY capability count before saving

["jiale yao" <19888972804@163.com>]

At 2026-09-29 11:32:33, "Baochen Qiang" <baochen.qiang@oss.qualcomm.com> wrote:
>
>
>On 9/25/2026 8:17 PM, Jiale Yao wrote:
>> Firmware supplies the hardware mode count and the PHY bitmap for each
>> mode in the service-ready event
>
>to be accurate, it is service ready ext event. we do have another event named service ready.
>
>> A mismatch
>> between the advertised total and the number of parsed capability TLVs can
>> also make the source pointer advance beyond its allocation.
>
>This claim doesn't hold. The source buffer mac_phy_caps is kzalloc'd for tot_phy_id
>elements, and the loop's mac_phy_cap++ runs exactly tot_phy_id times total, so the source
>pointer reads at most tot_phy_id entries — precisely the allocation bound, never past it.
>
>The n_mac_phy_caps > tot_phy_id case is already rejected at parse time in
>ath12k_wmi_mac_phy_caps_parse(). The only mismatch that can actually reach
>save_all_mac_phy_info() is n_mac_phy_caps < tot_phy_id, in which case the loop reads
>zeroed-but-allocated source entries and populates mac_phy_info with all-zero PHY info — a
>correctness bug, not a source-buffer overrun.
>
>> 
>> Validate both counts before writing any entries and propagate the error to
>> the service-ready parser.
>> 
>> Fixes: 062ade23991e ("wifi: ath12k: parse and save hardware mode info from WMI_SERVICE_READY_EXT_EVENTID event for later use")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jiale Yao <yaojiale02@163.com>
>> ---
>>  drivers/net/wireless/ath/ath12k/wmi.c | 22 ++++++++++++++++++++--
>>  1 file changed, 20 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c
>> index d5160af60e00..59ac17f0d48d 100644
>> --- a/drivers/net/wireless/ath/ath12k/wmi.c
>> +++ b/drivers/net/wireless/ath/ath12k/wmi.c
>> @@ -5004,7 +5004,7 @@ ath12k_wmi_save_mac_phy_info(struct ath12k_base *ab,
>>  					__le32_to_cpu(mac_phy_cap->high_5ghz_chan_freq);
>>  }
>>  
>> -static void
>> +static int
>>  ath12k_wmi_save_all_mac_phy_info(struct ath12k_base *ab,
>>  				 struct ath12k_wmi_svc_rdy_ext_parse *svc_rdy_ext)
>>  {
>> @@ -5015,6 +5015,20 @@ ath12k_wmi_save_all_mac_phy_info(struct ath12k_base *ab,
>>  	u32 hw_mode_id, phy_bit_map;
>>  	u8 hw_idx;
>>  
>> +	if (svc_rdy_ext->tot_phy_id > ARRAY_SIZE(svc_ext_info->mac_phy_info)) {
>> +		ath12k_warn(ab, "too many PHY entries %u (max %zu)\n",
>> +			    svc_rdy_ext->tot_phy_id,
>> +			    ARRAY_SIZE(svc_ext_info->mac_phy_info));
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (svc_rdy_ext->n_mac_phy_caps != svc_rdy_ext->tot_phy_id) {
>
>per comment above, should we use '<' instead of '!=' ?
>
>Besides, I think the right place for such check should be in
>ath12k_wmi_svc_rdy_ext_parse(), right before ath12k_wmi_save_all_mac_phy_info() and after
>mac phy cap parse ?

Thanks for your review, you are all right, how about this new one?

https://lore.kernel.org/all/20261003101944.596546-1-yaojiale02@163.com

>
>> +		ath12k_warn(ab, "invalid number of MAC/PHY caps %u, expected %u\n",
>> +			    svc_rdy_ext->n_mac_phy_caps,
>> +			    svc_rdy_ext->tot_phy_id);
>> +		return -EINVAL;
>> +	}
>> +
>>  	mac_phy_info = &svc_ext_info->mac_phy_info[0];
>>  	mac_phy_cap = svc_rdy_ext->mac_phy_caps;
>>  
>> @@ -5044,6 +5058,8 @@ ath12k_wmi_save_all_mac_phy_info(struct ath12k_base *ab,
>>  			phy_bit_map >>= 1;
>>  		}
>>  	}
>> +
>> +	return 0;
>>  }
>>  
>>  static int ath12k_wmi_svc_rdy_ext_parse(struct ath12k_base *ab,
>> @@ -5094,7 +5110,9 @@ static int ath12k_wmi_svc_rdy_ext_parse(struct ath12k_base *ab,
>>  				return ret;
>>  			}
>>  
>> -			ath12k_wmi_save_all_mac_phy_info(ab, svc_rdy_ext);
>> +			ret = ath12k_wmi_save_all_mac_phy_info(ab, svc_rdy_ext);
>> +			if (ret)
>> +				return ret;
>>  
>>  			svc_rdy_ext->mac_phy_done = true;
>>  		} else if (!svc_rdy_ext->ext_hal_reg_done) {