| Message ID | 20241207071306.325641-1-dheeraj.linuxdev@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,wireless-next] wifi: ath12k: Fix out-of-bounds read in ath12k_mac_vdev_create | expand |
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.
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
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
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
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
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",
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(+)