| Message ID | tencent_A64CA96B962349E369B349EA01EBC53C3505@qq.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ath11k: Fix potential RCU dereference issue in ath11k_debugfs_htt_ext_stats_handler | expand |
On 8/30/2024 5:02 AM, Jiawei Ye wrote: > In the `ath11k_debugfs_htt_ext_stats_handler` function, the `ar` pointer > obtained via RCU lock is accessed after the RCU read-side critical > section might be unlocked. According to RCU usage rules, this is illegal. > Reusing this pointer can lead to unpredictable behavior, including > accessing memory that has been updated or causing use-after-free issues. > The `ath12k_debugfs_htt_ext_stats_handler` function in the > `drivers/net/wireless/ath/ath12k/debugfs_htt_stats.c` file provides a good > example to follow for addressing this issue. > > This possible bug was identified using a static analysis tool developed > by myself, specifically designed to detect RCU-related issues. > > To address this issue, the RCU read lock is now kept until all accesses > to the `ar` pointer are completed. A `goto exit` statement is introduced > to ensure that the RCU read unlock is called appropriately, regardless of > the function's exit path. This analysis is incorrect since ar is not an RCU-protected structure The rcu_read_lock() is required internally within ath11k_mac_get_ar_by_pdev_id() when retrieving the RCU-protected pdev pointer. So NAK this patch.
diff --git a/drivers/net/wireless/ath/ath11k/debugfs_htt_stats.c b/drivers/net/wireless/ath/ath11k/debugfs_htt_stats.c index 870e86a31bf8..325377e00818 100644 --- a/drivers/net/wireless/ath/ath11k/debugfs_htt_stats.c +++ b/drivers/net/wireless/ath/ath11k/debugfs_htt_stats.c @@ -4572,15 +4572,14 @@ void ath11k_debugfs_htt_ext_stats_handler(struct ath11k_base *ab, pdev_id = FIELD_GET(HTT_STATS_COOKIE_LSB, cookie); rcu_read_lock(); ar = ath11k_mac_get_ar_by_pdev_id(ab, pdev_id); - rcu_read_unlock(); if (!ar) { ath11k_warn(ab, "failed to get ar for pdev_id %d\n", pdev_id); - return; + goto exit; } stats_req = ar->debug.htt_stats.stats_req; if (!stats_req) - return; + goto exit; spin_lock_bh(&ar->debug.htt_stats.lock); @@ -4599,6 +4598,8 @@ void ath11k_debugfs_htt_ext_stats_handler(struct ath11k_base *ab, if (send_completion) complete(&stats_req->cmpln); +exit: + rcu_read_unlock(); } static ssize_t ath11k_read_htt_stats_type(struct file *file,
In the `ath11k_debugfs_htt_ext_stats_handler` function, the `ar` pointer obtained via RCU lock is accessed after the RCU read-side critical section might be unlocked. According to RCU usage rules, this is illegal. Reusing this pointer can lead to unpredictable behavior, including accessing memory that has been updated or causing use-after-free issues. The `ath12k_debugfs_htt_ext_stats_handler` function in the `drivers/net/wireless/ath/ath12k/debugfs_htt_stats.c` file provides a good example to follow for addressing this issue. This possible bug was identified using a static analysis tool developed by myself, specifically designed to detect RCU-related issues. To address this issue, the RCU read lock is now kept until all accesses to the `ar` pointer are completed. A `goto exit` statement is introduced to ensure that the RCU read unlock is called appropriately, regardless of the function's exit path. Signed-off-by: Jiawei Ye <jiawei.ye@foxmail.com> --- drivers/net/wireless/ath/ath11k/debugfs_htt_stats.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)