| Message ID | 20181214035521.30388-1-baijiaju1990@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4f68ef64cd7feb1220232bd8f501d8aad340a099 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | cw1200: Fix concurrency use-after-free bugs in cw1200_hw_scan() | expand |
Jia-Ju Bai <baijiaju1990@gmail.com> wrote: > The function cw1200_bss_info_changed() and cw1200_hw_scan() can be > concurrently executed. > The two functions both access a possible shared variable "frame.skb". > > This shared variable is freed by dev_kfree_skb() in cw1200_upload_beacon(), > which is called by cw1200_bss_info_changed(). The free operation is > protected by a mutex lock "priv->conf_mutex" in cw1200_bss_info_changed(). > > In cw1200_hw_scan(), this shared variable is accessed without the > protection of the mutex lock "priv->conf_mutex". > Thus, concurrency use-after-free bugs may occur. > > To fix these bugs, the original calls to mutex_lock(&priv->conf_mutex) and > mutex_unlock(&priv->conf_mutex) are moved to the places, which can > protect the accesses to the shared variable. > > Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> Patch applied to wireless-drivers-next.git, thanks. 4f68ef64cd7f cw1200: Fix concurrency use-after-free bugs in cw1200_hw_scan()
diff --git a/drivers/net/wireless/st/cw1200/scan.c b/drivers/net/wireless/st/cw1200/scan.c index 67213f11acbd..0a9eac93dd01 100644 --- a/drivers/net/wireless/st/cw1200/scan.c +++ b/drivers/net/wireless/st/cw1200/scan.c @@ -78,6 +78,10 @@ int cw1200_hw_scan(struct ieee80211_hw *hw, if (req->n_ssids > WSM_SCAN_MAX_NUM_OF_SSIDS) return -EINVAL; + /* will be unlocked in cw1200_scan_work() */ + down(&priv->scan.lock); + mutex_lock(&priv->conf_mutex); + frame.skb = ieee80211_probereq_get(hw, priv->vif->addr, NULL, 0, req->ie_len); if (!frame.skb) @@ -86,19 +90,15 @@ int cw1200_hw_scan(struct ieee80211_hw *hw, if (req->ie_len) skb_put_data(frame.skb, req->ie, req->ie_len); - /* will be unlocked in cw1200_scan_work() */ - down(&priv->scan.lock); - mutex_lock(&priv->conf_mutex); - ret = wsm_set_template_frame(priv, &frame); if (!ret) { /* Host want to be the probe responder. */ ret = wsm_set_probe_responder(priv, true); } if (ret) { + dev_kfree_skb(frame.skb); mutex_unlock(&priv->conf_mutex); up(&priv->scan.lock); - dev_kfree_skb(frame.skb); return ret; } @@ -120,10 +120,9 @@ int cw1200_hw_scan(struct ieee80211_hw *hw, ++priv->scan.n_ssids; } - mutex_unlock(&priv->conf_mutex); - if (frame.skb) dev_kfree_skb(frame.skb); + mutex_unlock(&priv->conf_mutex); queue_work(priv->workqueue, &priv->scan.work); return 0; }
The function cw1200_bss_info_changed() and cw1200_hw_scan() can be concurrently executed. The two functions both access a possible shared variable "frame.skb". This shared variable is freed by dev_kfree_skb() in cw1200_upload_beacon(), which is called by cw1200_bss_info_changed(). The free operation is protected by a mutex lock "priv->conf_mutex" in cw1200_bss_info_changed(). In cw1200_hw_scan(), this shared variable is accessed without the protection of the mutex lock "priv->conf_mutex". Thus, concurrency use-after-free bugs may occur. To fix these bugs, the original calls to mutex_lock(&priv->conf_mutex) and mutex_unlock(&priv->conf_mutex) are moved to the places, which can protect the accesses to the shared variable. Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> --- drivers/net/wireless/st/cw1200/scan.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)