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

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

[Dheeraj Reddy Jonnalagadda]

Add a bounds check to ath12k_mac_vdev_create() to prevent an out-of-bounds
read in the vif->link_conf array. The function uses link_id, derived from
arvif->link_id, to index the array. When link_id equals 15, the index
exceeds the bounds of the array, which contains only 15 elements.

This issue occurs in the following code branch:

    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 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, where link_id is set to 15, causing an
out-of-bounds access when vif->link_conf array is read using link_id
as index.

Add a check to ensure that link_id does not exceed the valid range of the
vif->link_conf array. Log a warning and return -EINVAL if the check fails
to prevent undefined behavior.

Changelog:

v2:
	- Updated the commit message as per the reviewer's suggestions
	- Clarified the description of the bug in the commit message
	- Added Fixes and Closes tags with relevant information

Fixes: 90570ba4610 ("wifi: ath12k: do not return invalid link id for scan link")
Closes: https://scan7.scan.coverity.com/#/project-view/52337/11354?selectedIssue=1602214

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 v2 wireless-next] wifi: ath12k: Fix out-of-bounds read in ath12k_mac_vdev_create

[Kalle Valo]

Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com> writes:

> Add a bounds check to ath12k_mac_vdev_create() to prevent an out-of-bounds
> read in the vif->link_conf array. The function uses link_id, derived from
> arvif->link_id, to index the array. When link_id equals 15, the index
> exceeds the bounds of the array, which contains only 15 elements.
>
> This issue occurs in the following code branch:
>
>     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 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, where link_id is set to 15, causing an
> out-of-bounds access when vif->link_conf array is read using link_id
> as index.
>
> Add a check to ensure that link_id does not exceed the valid range of the
> vif->link_conf array. Log a warning and return -EINVAL if the check fails
> to prevent undefined behavior.
>
> Changelog:
>
> v2:
> 	- Updated the commit message as per the reviewer's suggestions
> 	- Clarified the description of the bug in the commit message
> 	- Added Fixes and Closes tags with relevant information
>
> Fixes: 90570ba4610 ("wifi: ath12k: do not return invalid link id for scan link")
> Closes: https://scan7.scan.coverity.com/#/project-view/52337/11354?selectedIssue=1602214
>
> Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com>
> ---

In wireless we prefer to have changelog after the '---' line so it's not
included in git. Not sure if Jeff can fix this during commit or not.

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

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

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

[Jeff Johnson]

On 12/6/2024 11:13 PM, Dheeraj Reddy Jonnalagadda wrote:
> Add a bounds check to ath12k_mac_vdev_create() to prevent an out-of-bounds
> read in the vif->link_conf array. The function uses link_id, derived from
> arvif->link_id, to index the array. When link_id equals 15, the index
> exceeds the bounds of the array, which contains only 15 elements.
> 
> This issue occurs in the following code branch:
> 
>     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 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, where link_id is set to 15, causing an
> out-of-bounds access when vif->link_conf array is read using link_id
> as index.
> 
> Add a check to ensure that link_id does not exceed the valid range of the
> vif->link_conf array. Log a warning and return -EINVAL if the check fails
> to prevent undefined behavior.
> 
> Changelog:
> 
> v2:
> 	- Updated the commit message as per the reviewer's suggestions
> 	- Clarified the description of the bug in the commit message
> 	- Added Fixes and Closes tags with relevant information

As Kalle already mentioned, the changelog should come "after the cut"
Please refer to:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format

Namely:
One good use for the additional comments after the --- marker is for a
diffstat...
Other comments relevant only to the moment or the maintainer, not suitable for
the permanent changelog, should also go here. A good example of such comments
might be patch changelogs which describe what has changed between the v1 and
v2 version of the patch.

> 
> Fixes: 90570ba4610 ("wifi: ath12k: do not return invalid link id for scan link")
> Closes: https://scan7.scan.coverity.com/#/project-view/52337/11354?selectedIssue=1602214

there should not be a blank line here

Also I just joined the Coverity "linux" project. I have access to the
dashboard, but don't see that particular CID. Since you've been a member for a
few months, is there something special I need to do to see that CID?
Or is this CID in a project other than "linux"? I ask because I'm looking at
A CID in the latest snapshot of "linux" and the URL is:
https://scan5.scan.coverity.com/#/project-view/63541/10063?selectedIssue=1636666

So I'm guessing your CID is from a different project?

> 
> Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com>
> ---

to reiterate, the changelog goes here, with the latest version described first.

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

Note that I don't need you to send a new patch if Kalle ACKs the current one;
I can fixup the patch in the "pending" branch. But I would like to make
sure I can see the CID in Coverity, so please let me know if I'm subscribed to
the correct project.

/jeff

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

[Dheeraj Reddy Jonnalagadda]

On Mon, Dec 09, 2024 at 10:39:36AM -0800, Jeff Johnson wrote:
> On 12/6/2024 11:13 PM, Dheeraj Reddy Jonnalagadda wrote:
> > Add a bounds check to ath12k_mac_vdev_create() to prevent an out-of-bounds
> > read in the vif->link_conf array. The function uses link_id, derived from
> > arvif->link_id, to index the array. When link_id equals 15, the index
> > exceeds the bounds of the array, which contains only 15 elements.
> > 
> > This issue occurs in the following code branch:
> > 
> >     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 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, where link_id is set to 15, causing an
> > out-of-bounds access when vif->link_conf array is read using link_id
> > as index.
> > 
> > Add a check to ensure that link_id does not exceed the valid range of the
> > vif->link_conf array. Log a warning and return -EINVAL if the check fails
> > to prevent undefined behavior.
> > 
> > Changelog:
> > 
> > v2:
> > 	- Updated the commit message as per the reviewer's suggestions
> > 	- Clarified the description of the bug in the commit message
> > 	- Added Fixes and Closes tags with relevant information
> 
> As Kalle already mentioned, the changelog should come "after the cut"
> Please refer to:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format
> 
> Namely:
> One good use for the additional comments after the --- marker is for a
> diffstat...
> Other comments relevant only to the moment or the maintainer, not suitable for
> the permanent changelog, should also go here. A good example of such comments
> might be patch changelogs which describe what has changed between the v1 and
> v2 version of the patch.
> 
> > 
> > Fixes: 90570ba4610 ("wifi: ath12k: do not return invalid link id for scan link")
> > Closes: https://scan7.scan.coverity.com/#/project-view/52337/11354?selectedIssue=1602214
> 
> there should not be a blank line here
> 
> Also I just joined the Coverity "linux" project. I have access to the
> dashboard, but don't see that particular CID. Since you've been a member for a
> few months, is there something special I need to do to see that CID?
> Or is this CID in a project other than "linux"? I ask because I'm looking at
> A CID in the latest snapshot of "linux" and the URL is:
> https://scan5.scan.coverity.com/#/project-view/63541/10063?selectedIssue=1636666
> 
> So I'm guessing your CID is from a different project?
> 
> > 
> > Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com>
> > ---
> 
> to reiterate, the changelog goes here, with the latest version described first.
> 
> >  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",
> 
> Note that I don't need you to send a new patch if Kalle ACKs the current one;
> I can fixup the patch in the "pending" branch. But I would like to make
> sure I can see the CID in Coverity, so please let me know if I'm subscribed to
> the correct project.
> 
> /jeff

Thank you for the feedback once again. The Coverity issue is from the
"linux-next weekly scan" project and you would have to be subscribed to it. 
The link to that project is here:
https://scan.coverity.com/projects/linux-next-weekly-scan?tab=overview

-Dheeraj

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

[Jeff Johnson]

On 12/9/2024 8:19 PM, Dheeraj Reddy Jonnalagadda wrote:
> Thank you for the feedback once again. The Coverity issue is from the
> "linux-next weekly scan" project and you would have to be subscribed to it. 
> The link to that project is here:
> https://scan.coverity.com/projects/linux-next-weekly-scan?tab=overview

Thanks, I found that project earlier today and joined.

So now I can track both linux and linux-next-weekly-scan.

/jeff

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

[Vasanthakumar Thiagarajan]

On 12/7/2024 12:43 PM, Dheeraj Reddy Jonnalagadda wrote:
> Add a bounds check to ath12k_mac_vdev_create() to prevent an out-of-bounds
> read in the vif->link_conf array. The function uses link_id, derived from
> arvif->link_id, to index the array. When link_id equals 15, the index
> exceeds the bounds of the array, which contains only 15 elements.
> 
> This issue occurs in the following code branch:
> 
>      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 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, where link_id is set to 15, causing an
> out-of-bounds access when vif->link_conf array is read using link_id
> as index.

Practically this can not happen as ath12k_mac_assign_link_vif() which
retrieves the arvif by link_id before calling ath12k_mac_vdev_create()
sanitizes the link_id when there are no valid_links running for that
vif (!vif->valid_links).

Vasanth