wifi: ath12k: Fix out-of-bounds read

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

Posted by Dheeraj Reddy Jonnalagadda 1 year ago

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
exceeds the bounds of the array, which contains only 15 elements.This
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
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).

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",
-- 
2.34.1

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

Posted by Jeff Johnson 1 year ago

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",

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

Posted by Kalle Valo 1 year ago

Jeff Johnson <jeff.johnson@oss.qualcomm.com> writes:

> 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()"

Nitpicking but sometimes I add the function name in front ('wifi:
ath12k: ath12k_mac_foo(): fix bar'), that way no need to add the 'in'
word. But this is just cosmetics, either way is fine.

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

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

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

Posted by Dheeraj Reddy Jonnalagadda 1 year ago

On Fri, Dec 06, 2024 at 12:06:51PM -0800, Jeff Johnson wrote:

Hi Jeff,
Thank you for taking the time to provide valuable feedback. I will make the
necessary changes and send the patch.

> 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?
In the code below, when the first condition in the if statement is true and 
the second condition is false, it implies that arvif->link_id equals 15 and 
the else branch is taken, therefore, assigning link_id to 15. The same
code path is shown by coverity. I will attach the link to the coverity report
to the updated patch.

	if (arvif->link_id == ATH12K_DEFAULT_SCAN_LINK && vif->valid_links)
                link_id = ffs(vif->valid_links) - 1;
        else
                link_id = arvif->link_id;

> 
> > 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",
>