[bug report] cw1200: Fix concurrency use-after-free bugs in cw1200_hw_scan()

[bug report] cw1200: Fix concurrency use-after-free bugs in cw1200_hw_scan()

[Dan Carpenter]

Hello Jia-Ju Bai,

The patch 4f68ef64cd7f: "cw1200: Fix concurrency use-after-free bugs
in cw1200_hw_scan()" from Dec 14, 2018, leads to the following static
checker warning:

drivers/net/wireless/st/cw1200/scan.c:127 cw1200_hw_scan() warn: inconsistent returns 'sem:&priv->scan.lock'.
  Locked on:   line 88
               line 127
  Unlocked on: line 66
               line 70
drivers/net/wireless/st/cw1200/scan.c:268 cw1200_scan_work() warn: inconsistent returns 'sem:&priv->scan.lock'.
  Locked on:   line 262
               line 268
  Unlocked on: line 196


(I don't really understand the locking in cw1200_scan_work() well enough
to say what's supposed to happen but the first warning seems like a bug).

drivers/net/wireless/st/cw1200/scan.c
    54 int cw1200_hw_scan(struct ieee80211_hw *hw,
    55 		   struct ieee80211_vif *vif,
    56 		   struct ieee80211_scan_request *hw_req)
    57 {
    58 	struct cw1200_common *priv = hw->priv;
    59 	struct cfg80211_scan_request *req = &hw_req->req;
    60 	struct wsm_template_frame frame = {
    61 		.frame_type = WSM_FRAME_TYPE_PROBE_REQUEST,
    62 	};
    63 	int i, ret;
    64 
    65 	if (!priv->vif)
    66 		return -EINVAL;
    67 
    68 	/* Scan when P2P_GO corrupt firmware MiniAP mode */
    69 	if (priv->join_status == CW1200_JOIN_STATUS_AP)
    70 		return -EOPNOTSUPP;
    71 
    72 	if (req->n_ssids == 1 && !req->ssids[0].ssid_len)
    73 		req->n_ssids = 0;
    74 
    75 	wiphy_dbg(hw->wiphy, "[SCAN] Scan request for %d SSIDs.\n",
    76 		  req->n_ssids);
    77 
    78 	if (req->n_ssids > WSM_SCAN_MAX_NUM_OF_SSIDS)
    79 		return -EINVAL;
    80 
    81 	/* will be unlocked in cw1200_scan_work() */
    82 	down(&priv->scan.lock);
        ^^^^^^^^^^^^^^^^^^^^^^
    83 	mutex_lock(&priv->conf_mutex);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    84 
    85 	frame.skb = ieee80211_probereq_get(hw, priv->vif->addr, NULL, 0,
    86 		req->ie_len);
    87 	if (!frame.skb)
    88 		return -ENOMEM;
                ^^^^^^^^^^^^^^^

Need to drop the locks before returning.

    89 
    90 	if (req->ie_len)
    91 		skb_put_data(frame.skb, req->ie, req->ie_len);
    92 
    93 	ret = wsm_set_template_frame(priv, &frame);
    94 	if (!ret) {
    95 		/* Host want to be the probe responder. */
    96 		ret = wsm_set_probe_responder(priv, true);
    97 	}
    98 	if (ret) {
    99 		dev_kfree_skb(frame.skb);
    100 		mutex_unlock(&priv->conf_mutex);
    101 		up(&priv->scan.lock);
    102 		return ret;
    103 	}
    104 
    105 	wsm_lock_tx(priv);
    106 
    107 	BUG_ON(priv->scan.req);
    108 	priv->scan.req = req;
    109 	priv->scan.n_ssids = 0;
    110 	priv->scan.status = 0;
    111 	priv->scan.begin = &req->channels[0];
    112 	priv->scan.curr = priv->scan.begin;
    113 	priv->scan.end = &req->channels[req->n_channels];
    114 	priv->scan.output_power = priv->output_power;
    115 
    116 	for (i = 0; i < req->n_ssids; ++i) {
    117 		struct wsm_ssid *dst = &priv->scan.ssids[priv->scan.n_ssids];
    118 		memcpy(&dst->ssid[0], req->ssids[i].ssid, sizeof(dst->ssid));
    119 		dst->length = req->ssids[i].ssid_len;
    120 		++priv->scan.n_ssids;
    121 	}
    122 
    123 	if (frame.skb)
    124 		dev_kfree_skb(frame.skb);
    125 	mutex_unlock(&priv->conf_mutex);
    126 	queue_work(priv->workqueue, &priv->scan.work);
    127 	return 0;
    128 }

regards,
dan carpenter