wifi: ath12k: Fix out-of-bounds read
PATCH 1/1 wifi: ath12k: Fix out-of-bounds read
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).
--- 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
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()"
Quoted 4 lines
This patch addresses the Out-of-bounds read issue detected by Coverity (CID 1602214). The function
ath12k_mac_vdev_create()accesses thevif->link_confarray usinglink_id, which is derived fromarvif->link_id. In cases wherearvif->link_idequals 15, the index
How can arvif->link_id equal 15? Does Coverity actually identify a code path where this can occur?
Quoted 1 line
exceeds the bounds of the array, which contains only 15 elements.This
nit: space after .
Quoted 9 lines
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;
Quoted 4 lines
When
arvif->link_idequals 15 and the else branch is taken,link_idis set to 15.This patch adds a bounds check to ensure that
link_iddoes 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...
Quoted 2 lines
the valid range of the
vif->link_confarray. 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)
Quoted 1 line
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>
Quoted 22 lines
Signed-off-by Dheeraj Reddy Jonnalagadda <[email protected]>--- 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",
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.
Quoted 6 lines · Dheeraj Reddy Jonnalagadda
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()"
Quoted 7 lines
Quoted 4 lines
This patch addresses the Out-of-bounds read issue detected by Coverity (CID 1602214). The function
ath12k_mac_vdev_create()accesses thevif->link_confarray usinglink_id, which is derived fromarvif->link_id. In cases wherearvif->link_idequals 15, the indexHow can
arvif->link_idequal 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;Quoted 64 lines
Quoted 2 lines
exceeds the bounds of the array, which contains only 15 elements.This
nit: space after .
Quoted 9 lines
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;Quoted 4 lines
When
arvif->link_idequals 15 and the else branch is taken,link_idis set to 15.This patch adds a bounds check to ensure that
link_iddoes not exceedSee
<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...
Quoted 2 lines
the valid range of the
vif->link_confarray. 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)
Quoted 1 line
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>Quoted 22 lines
Signed-off-by Dheeraj Reddy Jonnalagadda <[email protected]>--- 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",
Quoted 5 lines · Jeff Johnson
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.
Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches