cfg80211: fix management registrations locking

The management registrations locking was broken, the list was
locked for each wdev, but cfg80211_mgmt_registrations_update()
iterated it without holding all the correct spinlocks, causing
list corruption.

Rather than trying to fix it with fine-grained locking, just
move the lock to the wiphy/rdev (still need the list on each
wdev), we already need to hold the wdev lock to change it, so
there's no contention on the lock in any case. This trivially
fixes the bug since we hold one wdev's lock already, and now
will hold the lock that protects all lists.

Cc: stable@vger.kernel.org
Reported-by: Jouni Malinen <j@w1.fi>
Fixes: 6cd536fe62ef ("cfg80211: change internal management frame registration API")
Link: https://lore.kernel.org/r/20211025133111.5cf733eab0f4.I7b0abb0494ab712f74e2efcd24bb31ac33f7eee9@changeid
Signed-off-by: Johannes Berg <johannes.berg@intel.com>

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 62dd8422e0dca3a8da4f679ddfd57a240deac69a..27336fc7046749f4be37337f4dfd5dce70ce0ff2 100644 (file)
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -5376,7 +5376,6 @@ static inline void wiphy_unlock(struct wiphy *wiphy)
  *     netdev and may otherwise be used by driver read-only, will be update
  *     by cfg80211 on change_interface
  * @mgmt_registrations: list of registrations for management frames
- * @mgmt_registrations_lock: lock for the list
  * @mgmt_registrations_need_update: mgmt registrations were updated,
  *     need to propagate the update to the driver
  * @mtx: mutex used to lock data in this struct, may be used by drivers
@@ -5423,7 +5422,6 @@ struct wireless_dev {
        u32 identifier;
 
        struct list_head mgmt_registrations;
-       spinlock_t mgmt_registrations_lock;
        u8 mgmt_registrations_need_update:1;
 
        struct mutex mtx;

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 03323121ca505c0d7c46a1cbfde94d1c2cfc9c5c..aaba847d79eb296127a91545a9d97fcb503a3918 100644 (file)
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -524,6 +524,7 @@ use_default_name:
        INIT_WORK(&rdev->propagate_cac_done_wk, cfg80211_propagate_cac_done_wk);
        INIT_WORK(&rdev->mgmt_registrations_update_wk,
                  cfg80211_mgmt_registrations_update_wk);
+       spin_lock_init(&rdev->mgmt_registrations_lock);
 
 #ifdef CONFIG_CFG80211_DEFAULT_PS
        rdev->wiphy.flags |= WIPHY_FLAG_PS_ON_BY_DEFAULT;
@@ -1279,7 +1280,6 @@ void cfg80211_init_wdev(struct wireless_dev *wdev)
        INIT_LIST_HEAD(&wdev->event_list);
        spin_lock_init(&wdev->event_lock);
        INIT_LIST_HEAD(&wdev->mgmt_registrations);
-       spin_lock_init(&wdev->mgmt_registrations_lock);
        INIT_LIST_HEAD(&wdev->pmsr_list);
        spin_lock_init(&wdev->pmsr_lock);
        INIT_WORK(&wdev->pmsr_free_wk, cfg80211_pmsr_free_wk);

diff --git a/net/wireless/core.h b/net/wireless/core.h
index b35d0db12f1d5c2abd7911f2d1593f574b187372..1720abf36f92a314a329f46d8bc07ba85a4ef241 100644 (file)
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -100,6 +100,8 @@ struct cfg80211_registered_device {
        struct work_struct propagate_cac_done_wk;
 
        struct work_struct mgmt_registrations_update_wk;
+       /* lock for all wdev lists */
+       spinlock_t mgmt_registrations_lock;
 
        /* must be last because of the way we do wiphy_priv(),
         * and it should at least be aligned to NETDEV_ALIGN */

diff --git a/net/wireless/mlme.c b/net/wireless/mlme.c
index 3aa69b375a107a4f921b3dde08b392b6e6348327..783acd2c4211f804dc9402ff23e3abeb7a695e1a 100644 (file)
--- a/net/wireless/mlme.c
+++ b/net/wireless/mlme.c
@@ -452,9 +452,9 @@ static void cfg80211_mgmt_registrations_update(struct wireless_dev *wdev)
 
        lockdep_assert_held(&rdev->wiphy.mtx);
 
-       spin_lock_bh(&wdev->mgmt_registrations_lock);
+       spin_lock_bh(&rdev->mgmt_registrations_lock);
        if (!wdev->mgmt_registrations_need_update) {
-               spin_unlock_bh(&wdev->mgmt_registrations_lock);
+               spin_unlock_bh(&rdev->mgmt_registrations_lock);
                return;
        }
 
@@ -479,7 +479,7 @@ static void cfg80211_mgmt_registrations_update(struct wireless_dev *wdev)
        rcu_read_unlock();
 
        wdev->mgmt_registrations_need_update = 0;
-       spin_unlock_bh(&wdev->mgmt_registrations_lock);
+       spin_unlock_bh(&rdev->mgmt_registrations_lock);
 
        rdev_update_mgmt_frame_registrations(rdev, wdev, &upd);
 }
@@ -503,6 +503,7 @@ int cfg80211_mlme_register_mgmt(struct wireless_dev *wdev, u32 snd_portid,
                                int match_len, bool multicast_rx,
                                struct netlink_ext_ack *extack)
 {
+       struct cfg80211_registered_device *rdev = wiphy_to_rdev(wdev->wiphy);
        struct cfg80211_mgmt_registration *reg, *nreg;
        int err = 0;
        u16 mgmt_type;
@@ -548,7 +549,7 @@ int cfg80211_mlme_register_mgmt(struct wireless_dev *wdev, u32 snd_portid,
        if (!nreg)
                return -ENOMEM;
 
-       spin_lock_bh(&wdev->mgmt_registrations_lock);
+       spin_lock_bh(&rdev->mgmt_registrations_lock);
 
        list_for_each_entry(reg, &wdev->mgmt_registrations, list) {
                int mlen = min(match_len, reg->match_len);
@@ -583,7 +584,7 @@ int cfg80211_mlme_register_mgmt(struct wireless_dev *wdev, u32 snd_portid,
                list_add(&nreg->list, &wdev->mgmt_registrations);
        }
        wdev->mgmt_registrations_need_update = 1;
-       spin_unlock_bh(&wdev->mgmt_registrations_lock);
+       spin_unlock_bh(&rdev->mgmt_registrations_lock);
 
        cfg80211_mgmt_registrations_update(wdev);
 
@@ -591,7 +592,7 @@ int cfg80211_mlme_register_mgmt(struct wireless_dev *wdev, u32 snd_portid,
 
  out:
        kfree(nreg);
-       spin_unlock_bh(&wdev->mgmt_registrations_lock);
+       spin_unlock_bh(&rdev->mgmt_registrations_lock);
 
        return err;
 }
@@ -602,7 +603,7 @@ void cfg80211_mlme_unregister_socket(struct wireless_dev *wdev, u32 nlportid)
        struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
        struct cfg80211_mgmt_registration *reg, *tmp;
 
-       spin_lock_bh(&wdev->mgmt_registrations_lock);
+       spin_lock_bh(&rdev->mgmt_registrations_lock);
 
        list_for_each_entry_safe(reg, tmp, &wdev->mgmt_registrations, list) {
                if (reg->nlportid != nlportid)
@@ -615,7 +616,7 @@ void cfg80211_mlme_unregister_socket(struct wireless_dev *wdev, u32 nlportid)
                schedule_work(&rdev->mgmt_registrations_update_wk);
        }
 
-       spin_unlock_bh(&wdev->mgmt_registrations_lock);
+       spin_unlock_bh(&rdev->mgmt_registrations_lock);
 
        if (nlportid && rdev->crit_proto_nlportid == nlportid) {
                rdev->crit_proto_nlportid = 0;
@@ -628,15 +629,16 @@ void cfg80211_mlme_unregister_socket(struct wireless_dev *wdev, u32 nlportid)
 
 void cfg80211_mlme_purge_registrations(struct wireless_dev *wdev)
 {
+       struct cfg80211_registered_device *rdev = wiphy_to_rdev(wdev->wiphy);
        struct cfg80211_mgmt_registration *reg, *tmp;
 
-       spin_lock_bh(&wdev->mgmt_registrations_lock);
+       spin_lock_bh(&rdev->mgmt_registrations_lock);
        list_for_each_entry_safe(reg, tmp, &wdev->mgmt_registrations, list) {
                list_del(&reg->list);
                kfree(reg);
        }
        wdev->mgmt_registrations_need_update = 1;
-       spin_unlock_bh(&wdev->mgmt_registrations_lock);
+       spin_unlock_bh(&rdev->mgmt_registrations_lock);
 
        cfg80211_mgmt_registrations_update(wdev);
 }
@@ -784,7 +786,7 @@ bool cfg80211_rx_mgmt_khz(struct wireless_dev *wdev, int freq, int sig_dbm,
        data = buf + ieee80211_hdrlen(mgmt->frame_control);
        data_len = len - ieee80211_hdrlen(mgmt->frame_control);
 
-       spin_lock_bh(&wdev->mgmt_registrations_lock);
+       spin_lock_bh(&rdev->mgmt_registrations_lock);
 
        list_for_each_entry(reg, &wdev->mgmt_registrations, list) {
                if (reg->frame_type != ftype)
@@ -808,7 +810,7 @@ bool cfg80211_rx_mgmt_khz(struct wireless_dev *wdev, int freq, int sig_dbm,
                break;
        }
 
-       spin_unlock_bh(&wdev->mgmt_registrations_lock);
+       spin_unlock_bh(&rdev->mgmt_registrations_lock);
 
        trace_cfg80211_return_bool(result);
        return result;