On 2/29/2024 12:40 AM, Dmitry Antipov wrote:
> Since 'ath11k_mac_get_ar_by_pdev_id()' can return NULL, check
> the return value in 'ath11k_wmi_tlv_rssi_chain_parse()' as well
> as in 'ath11k_wmi_tlv_fw_stats_data_parse()', and return -EINVAL
> in case of error. Compile tested only.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Signed-off-by: Dmitry Antipov <dmantipov@xxxxxxxxx>
> ---
> v2: aggregate to the series
In the future please don't use In-Reply-To: when submitting a new
version of a patch/series. Each version should start its own thread.
Having In-Reference-To: is OK
> ---
> drivers/net/wireless/ath/ath11k/wmi.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath11k/wmi.c b/drivers/net/wireless/ath/ath11k/wmi.c
> index 34ab9631ff36..2d93e4e78a37 100644
> --- a/drivers/net/wireless/ath/ath11k/wmi.c
> +++ b/drivers/net/wireless/ath/ath11k/wmi.c
> @@ -6498,6 +6498,12 @@ static int ath11k_wmi_tlv_rssi_chain_parse(struct ath11k_base *ab,
> rcu_read_lock();
>
> ar = ath11k_mac_get_ar_by_pdev_id(ab, ev->pdev_id);
> + if (!ar) {
> + ath11k_warn(ab, "%s: invalid pdev_id %d\n",
> + __func__, ev->pdev_id);
it is currently not the convention to use __func__ in ath11k logs.
there are currently 6 instances where the NULL check is present, and
each has a custom log message
I personally would be ok having this new format, but want to get Kalle's
approval for that. But if we do that I'd also want a follow-up patch to
modify those 6 existing logs.
FWIW in all of ath11k there are only 3 instances of __func__:
mac.c: "%s: rsn ie found\n", __func__);
mac.c: "%s: wpa ie found\n", __func__);
wmi.c: __func__);
> + ret = -EINVAL;
> + goto exit;
> + }
> stats->stats_id = WMI_REQUEST_RSSI_PER_CHAIN_STAT;
>
> ath11k_dbg(ab, ATH11K_DBG_WMI,
> @@ -6570,6 +6576,12 @@ static int ath11k_wmi_tlv_fw_stats_data_parse(struct ath11k_base *ab,
> rcu_read_lock();
>
> ar = ath11k_mac_get_ar_by_pdev_id(ab, ev->pdev_id);
> + if (!ar) {
> + ath11k_warn(ab, "%s: invalid pdev_id %d\n",
> + __func__, ev->pdev_id);
> + ret = -EINVAL;
> + goto exit;
> + }
>
> for (i = 0; i < ev->num_pdev_stats; i++) {
> const struct wmi_pdev_stats *src;
The actual logic is correct.
/jeff
