Re: [PATCH wireless-next] wifi: ath12k: Fix out-of-bounds read

[Jeff Johnson <jeff.johnson@oss.qualcomm.com>]

On 12/5/2024 11:35 PM, Dheeraj Reddy Jonnalagadda wrote:

The subject should be as specific as possible while still fitting on one line.
Ideally the subject should be unique. So at a minimum I'd add "in
ath12k_mac_vdev_create()"

> This patch addresses the Out-of-bounds read issue detected by
> Coverity (CID 1602214). The function ath12k_mac_vdev_create() accesses
> the vif->link_conf array using link_id, which is derived from
> arvif->link_id. In cases where arvif->link_id equals 15, the index

How can arvif->link_id equal 15? Does Coverity actually identify a code path
where this can occur?

> exceeds the bounds of the array, which contains only 15 elements.This

nit: space after .

> results in an out-of-bounds read.
> 
> This issue occurs in the following branch of the code:
> 
>     if (arvif->link_id == ATH12K_DEFAULT_SCAN_LINK && vif->valid_links)
>         link_id = ffs(vif->valid_links) - 1;
>     else
>         link_id = arvif->link_id;
> 
> When arvif->link_id equals 15 and the else branch is taken, link_id is
> set to 15.
> 
> This patch adds a bounds check to ensure that link_id does not exceed

See
<https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes>
and specifically:
<quote>
Describe your changes in imperative mood, e.g. “make xyzzy do frotz” instead
of “[This patch] makes xyzzy do frotz” or “[I] changed xyzzy to do frotz”, as
if you are giving orders to the codebase to change its behaviour.
</quote>

So this should start: "Add a bounds check...

> the valid range of the vif->link_conf array. If the check fails, a
> warning is logged, and the function returns an error code (-EINVAL).

again use imperative mood (log a warning, return an error)

> 

Prior to the SOB you should at least have two other tags:
Fixes: <refer to the patch that introduced the bug>
Closes: <the public url of the coverity report>

> Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com>
> ---
>  drivers/net/wireless/ath/ath12k/mac.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> index 129607ac6c1a..c19b10e66f4a 100644
> --- a/drivers/net/wireless/ath/ath12k/mac.c
> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> @@ -7725,6 +7725,12 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif)
>  	else
>  		link_id = arvif->link_id;
>  
> +	if (link_id >= ARRAY_SIZE(vif->link_conf)) {
> +		ath12k_warn(ar->ab, "link_id %u exceeds max valid links for vif %pM\n",
> +			    link_id, vif->addr);
> +		return -EINVAL;
> +	}
> +
>  	link_conf = wiphy_dereference(hw->wiphy, vif->link_conf[link_id]);
>  	if (!link_conf) {
>  		ath12k_warn(ar->ab, "unable to access bss link conf in vdev create for vif %pM link %u\n",