| Message ID | 20241206073542.315095-1-dheeraj.linuxdev@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [wireless-next] wifi: ath12k: Fix out-of-bounds read | expand |
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",
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", >
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.
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",
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(+)