wifi: ath12k: Fix out-of-bounds read in ath12k_mac_vdev_create
PATCH v2 1/1 wifi: ath12k: Fix out-of-bounds read in ath12k_mac_vdev_create
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
--- drivers/net/wireless/ath/ath12k/mac.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/net/wireless/ath/ath12k/mac.c +6 -0
@@ -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",
Signature · 1 line
2.34.1
Quoted 29 lines · Dheeraj Reddy Jonnalagadda
Add a bounds check to
ath12k_mac_vdev_create()to prevent an out-of-bounds read in thevif->link_confarray. The function useslink_id, derived fromarvif->link_id, to index the array. Whenlink_idequals 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_idequals 15 and the else branch is taken, wherelink_idis set to 15, causing an out-of-bounds access whenvif->link_confarray is read usinglink_idas index.Add a check to ensure that
link_iddoes not exceed the valid range of thevif->link_confarray. Log a warning and return-EINVALif 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
Quoted 5 lines
Fixes 90570ba4610 ("wifi: ath12k: do not return invalid link id for scan link")Signed-off-by Dheeraj Reddy Jonnalagadda <[email protected]>---
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.
Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Quoted 22 lines · Dheeraj Reddy Jonnalagadda
Add a bounds check to
ath12k_mac_vdev_create()to prevent an out-of-bounds read in thevif->link_confarray. The function useslink_id, derived fromarvif->link_id, to index the array. Whenlink_idequals 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_idequals 15 and the else branch is taken, wherelink_idis set to 15, causing an out-of-bounds access whenvif->link_confarray is read usinglink_idas index.Add a check to ensure that
link_iddoes not exceed the valid range of thevif->link_confarray. Log a warning and return-EINVALif the check fails to prevent undefined behavior.
Quoted 6 lines
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.
Quoted 3 lines
Fixes 90570ba4610 ("wifi: ath12k: do not return invalid link id for scan link")
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?
Quoted 3 lines
Signed-off-by Dheeraj Reddy Jonnalagadda <[email protected]>---
to reiterate, the changelog goes here, with the latest version described first.
Quoted 10 lines
drivers/net/wireless/ath/ath12k/mac.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/net/wireless/ath/ath12k/mac.c +0
-0@@ -7725,6 +7725,12 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif)else link_id = arvif->link_id;
drivers/net/wireless/ath/ath12k/mac.c int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif) · line 7736
Quoted 10 lines
drivers/net/wireless/ath/ath12k/mac.c +6
-0+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
Quoted 89 lines · Jeff Johnson
Quoted 22 lines · Dheeraj Reddy Jonnalagadda
Add a bounds check to
ath12k_mac_vdev_create()to prevent an out-of-bounds read in thevif->link_confarray. The function useslink_id, derived fromarvif->link_id, to index the array. Whenlink_idequals 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_idequals 15 and the else branch is taken, wherelink_idis set to 15, causing an out-of-bounds access whenvif->link_confarray is read usinglink_idas index.Add a check to ensure that
link_iddoes not exceed the valid range of thevif->link_confarray. Log a warning and return-EINVALif the check fails to prevent undefined behavior.Quoted 6 lines
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 informationAs 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-formatNamely:
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.Quoted 3 lines
Fixes 90570ba4610 ("wifi: ath12k: do not return invalid link id for scan link")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=1636666So I'm guessing your CID is from a different project?
Quoted 3 lines
Signed-off-by Dheeraj Reddy Jonnalagadda <[email protected]>---
to reiterate, the changelog goes here, with the latest version described first.
Quoted 10 lines
drivers/net/wireless/ath/ath12k/mac.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/net/wireless/ath/ath12k/mac.c +0
-0@@ -7725,6 +7725,12 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif)else link_id = arvif->link_id;drivers/net/wireless/ath/ath12k/mac.c int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif) · line 7736
Quoted 10 lines
drivers/net/wireless/ath/ath12k/mac.c +6
-0+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.
Quoted 1 line
/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
Quoted 4 lines · Dheeraj Reddy Jonnalagadda
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
Quoted 12 lines · Dheeraj Reddy Jonnalagadda
Add a bounds check to
ath12k_mac_vdev_create()to prevent an out-of-bounds read in thevif->link_confarray. The function useslink_id, derived fromarvif->link_id, to index the array. Whenlink_idequals 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;
Quoted 5 lines
When the first condition in the if statement is true and the second condition is false, it implies that
arvif->link_idequals 15 and the else branch is taken, wherelink_idis set to 15, causing an out-of-bounds access whenvif->link_confarray is read usinglink_idas 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