Skip to content

Commit

Permalink
wifi: cfg80211: fix cqm_config access race
Browse files Browse the repository at this point in the history 
Max Schulze reports crashes with brcmfmac. The reason seems
to be a race between userspace removing the CQM config and
the driver calling cfg80211_cqm_rssi_notify(), where if the
data is freed while cfg80211_cqm_rssi_notify() runs it will
crash since it assumes wdev->cqm_config is set. This can't
be fixed with a simple non-NULL check since there's nothing
we can do for locking easily, so use RCU instead to protect
the pointer, but that requires pulling the updates out into
an asynchronous worker so they can sleep and call back into
the driver.

Since we need to change the free anyway, also change it to
go back to the old settings if changing the settings fails.

Reported-and-tested-by: Max Schulze <max.schulze@online.de>
Closes: https://lore.kernel.org/r/ac96309a-8d8d-4435-36e6-6d152eb31876@online.de
Fixes: 4a4b816 ("cfg80211: Accept multiple RSSI thresholds for CQM")
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
  • Loading branch information
Johannes Berg committed Sep 11, 2023
1 parent 8ba438e commit 37c20b2
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 42 deletions.
3 changes: 2 additions & 1 deletion include/net/cfg80211.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6013,7 +6013,8 @@ struct wireless_dev {
} wext;
#endif

struct cfg80211_cqm_config *cqm_config;
struct wiphy_work cqm_rssi_work;
struct cfg80211_cqm_config __rcu *cqm_config;

struct list_head pmsr_list;
spinlock_t pmsr_lock;
Expand Down
14 changes: 7 additions & 7 deletions net/wireless/core.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1181,16 +1181,11 @@ void wiphy_rfkill_set_hw_state_reason(struct wiphy *wiphy, bool blocked,
}
EXPORT_SYMBOL(wiphy_rfkill_set_hw_state_reason);

void cfg80211_cqm_config_free(struct wireless_dev *wdev)
{
kfree(wdev->cqm_config);
wdev->cqm_config = NULL;
}

static void _cfg80211_unregister_wdev(struct wireless_dev *wdev,
bool unregister_netdev)
{
struct cfg80211_registered_device *rdev = wiphy_to_rdev(wdev->wiphy);
struct cfg80211_cqm_config *cqm_config;
unsigned int link_id;

ASSERT_RTNL();
Expand Down Expand Up @@ -1227,7 +1222,10 @@ static void _cfg80211_unregister_wdev(struct wireless_dev *wdev,
kfree_sensitive(wdev->wext.keys);
wdev->wext.keys = NULL;
#endif
cfg80211_cqm_config_free(wdev);
wiphy_work_cancel(wdev->wiphy, &wdev->cqm_rssi_work);
/* deleted from the list, so can't be found from nl80211 any more */
cqm_config = rcu_access_pointer(wdev->cqm_config);
kfree_rcu(cqm_config, rcu_head);

/*
* Ensure that all events have been processed and
Expand Down Expand Up @@ -1379,6 +1377,8 @@ void cfg80211_init_wdev(struct wireless_dev *wdev)
wdev->wext.connect.auth_type = NL80211_AUTHTYPE_AUTOMATIC;
#endif

wiphy_work_init(&wdev->cqm_rssi_work, cfg80211_cqm_rssi_notify_work);

if (wdev->wiphy->flags & WIPHY_FLAG_PS_ON_BY_DEFAULT)
wdev->ps = true;
else
Expand Down
7 changes: 5 additions & 2 deletions net/wireless/core.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -295,12 +295,17 @@ struct cfg80211_beacon_registration {
};

struct cfg80211_cqm_config {
struct rcu_head rcu_head;
u32 rssi_hyst;
s32 last_rssi_event_value;
enum nl80211_cqm_rssi_threshold_event last_rssi_event_type;
int n_rssi_thresholds;
s32 rssi_thresholds[] __counted_by(n_rssi_thresholds);
};

void cfg80211_cqm_rssi_notify_work(struct wiphy *wiphy,
struct wiphy_work *work);

void cfg80211_destroy_ifaces(struct cfg80211_registered_device *rdev);

/* free object */
Expand Down Expand Up @@ -566,8 +571,6 @@ cfg80211_bss_update(struct cfg80211_registered_device *rdev,
#define CFG80211_DEV_WARN_ON(cond) ({bool __r = (cond); __r; })
#endif

void cfg80211_cqm_config_free(struct wireless_dev *wdev);

void cfg80211_release_pmsr(struct wireless_dev *wdev, u32 portid);
void cfg80211_pmsr_wdev_down(struct wireless_dev *wdev);
void cfg80211_pmsr_free_wk(struct work_struct *work);
Expand Down
93 changes: 61 additions & 32 deletions net/wireless/nl80211.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12815,7 +12815,8 @@ static int nl80211_set_cqm_txe(struct genl_info *info,
}

static int cfg80211_cqm_rssi_update(struct cfg80211_registered_device *rdev,
struct net_device *dev)
struct net_device *dev,
struct cfg80211_cqm_config *cqm_config)
{
struct wireless_dev *wdev = dev->ieee80211_ptr;
s32 last, low, high;
Expand All @@ -12824,7 +12825,7 @@ static int cfg80211_cqm_rssi_update(struct cfg80211_registered_device *rdev,
int err;

/* RSSI reporting disabled? */
if (!wdev->cqm_config)
if (!cqm_config)
return rdev_set_cqm_rssi_range_config(rdev, dev, 0, 0);

/*
Expand All @@ -12833,7 +12834,7 @@ static int cfg80211_cqm_rssi_update(struct cfg80211_registered_device *rdev,
* connection is established and enough beacons received to calculate
* the average.
*/
if (!wdev->cqm_config->last_rssi_event_value &&
if (!cqm_config->last_rssi_event_value &&
wdev->links[0].client.current_bss &&
rdev->ops->get_station) {
struct station_info sinfo = {};
Expand All @@ -12847,30 +12848,30 @@ static int cfg80211_cqm_rssi_update(struct cfg80211_registered_device *rdev,

cfg80211_sinfo_release_content(&sinfo);
if (sinfo.filled & BIT_ULL(NL80211_STA_INFO_BEACON_SIGNAL_AVG))
wdev->cqm_config->last_rssi_event_value =
cqm_config->last_rssi_event_value =
(s8) sinfo.rx_beacon_signal_avg;
}

last = wdev->cqm_config->last_rssi_event_value;
hyst = wdev->cqm_config->rssi_hyst;
n = wdev->cqm_config->n_rssi_thresholds;
last = cqm_config->last_rssi_event_value;
hyst = cqm_config->rssi_hyst;
n = cqm_config->n_rssi_thresholds;

for (i = 0; i < n; i++) {
i = array_index_nospec(i, n);
if (last < wdev->cqm_config->rssi_thresholds[i])
if (last < cqm_config->rssi_thresholds[i])
break;
}

low_index = i - 1;
if (low_index >= 0) {
low_index = array_index_nospec(low_index, n);
low = wdev->cqm_config->rssi_thresholds[low_index] - hyst;
low = cqm_config->rssi_thresholds[low_index] - hyst;
} else {
low = S32_MIN;
}
if (i < n) {
i = array_index_nospec(i, n);
high = wdev->cqm_config->rssi_thresholds[i] + hyst - 1;
high = cqm_config->rssi_thresholds[i] + hyst - 1;
} else {
high = S32_MAX;
}
Expand All @@ -12883,6 +12884,7 @@ static int nl80211_set_cqm_rssi(struct genl_info *info,
u32 hysteresis)
{
struct cfg80211_registered_device *rdev = info->user_ptr[0];
struct cfg80211_cqm_config *cqm_config = NULL, *old;
struct net_device *dev = info->user_ptr[1];
struct wireless_dev *wdev = dev->ieee80211_ptr;
int i, err;
Expand All @@ -12900,10 +12902,6 @@ static int nl80211_set_cqm_rssi(struct genl_info *info,
wdev->iftype != NL80211_IFTYPE_P2P_CLIENT)
return -EOPNOTSUPP;

wdev_lock(wdev);
cfg80211_cqm_config_free(wdev);
wdev_unlock(wdev);

if (n_thresholds <= 1 && rdev->ops->set_cqm_rssi_config) {
if (n_thresholds == 0 || thresholds[0] == 0) /* Disabling */
return rdev_set_cqm_rssi_config(rdev, dev, 0, 0);
Expand All @@ -12920,9 +12918,10 @@ static int nl80211_set_cqm_rssi(struct genl_info *info,
n_thresholds = 0;

wdev_lock(wdev);
if (n_thresholds) {
struct cfg80211_cqm_config *cqm_config;
old = rcu_dereference_protected(wdev->cqm_config,
lockdep_is_held(&wdev->mtx));

if (n_thresholds) {
cqm_config = kzalloc(struct_size(cqm_config, rssi_thresholds,
n_thresholds),
GFP_KERNEL);
Expand All @@ -12937,11 +12936,18 @@ static int nl80211_set_cqm_rssi(struct genl_info *info,
flex_array_size(cqm_config, rssi_thresholds,
n_thresholds));

wdev->cqm_config = cqm_config;
rcu_assign_pointer(wdev->cqm_config, cqm_config);
} else {
RCU_INIT_POINTER(wdev->cqm_config, NULL);
}

err = cfg80211_cqm_rssi_update(rdev, dev);

err = cfg80211_cqm_rssi_update(rdev, dev, cqm_config);
if (err) {
rcu_assign_pointer(wdev->cqm_config, old);
kfree_rcu(cqm_config, rcu_head);
} else {
kfree_rcu(old, rcu_head);
}
unlock:
wdev_unlock(wdev);

Expand Down Expand Up @@ -19092,28 +19098,50 @@ void cfg80211_cqm_rssi_notify(struct net_device *dev,
enum nl80211_cqm_rssi_threshold_event rssi_event,
s32 rssi_level, gfp_t gfp)
{
struct sk_buff *msg;
struct wireless_dev *wdev = dev->ieee80211_ptr;
struct cfg80211_registered_device *rdev = wiphy_to_rdev(wdev->wiphy);
struct cfg80211_cqm_config *cqm_config;

trace_cfg80211_cqm_rssi_notify(dev, rssi_event, rssi_level);

if (WARN_ON(rssi_event != NL80211_CQM_RSSI_THRESHOLD_EVENT_LOW &&
rssi_event != NL80211_CQM_RSSI_THRESHOLD_EVENT_HIGH))
return;

if (wdev->cqm_config) {
wdev->cqm_config->last_rssi_event_value = rssi_level;
rcu_read_lock();
cqm_config = rcu_dereference(wdev->cqm_config);
if (cqm_config) {
cqm_config->last_rssi_event_value = rssi_level;
cqm_config->last_rssi_event_type = rssi_event;
wiphy_work_queue(wdev->wiphy, &wdev->cqm_rssi_work);
}
rcu_read_unlock();
}
EXPORT_SYMBOL(cfg80211_cqm_rssi_notify);

void cfg80211_cqm_rssi_notify_work(struct wiphy *wiphy, struct wiphy_work *work)
{
struct wireless_dev *wdev = container_of(work, struct wireless_dev,
cqm_rssi_work);
struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
enum nl80211_cqm_rssi_threshold_event rssi_event;
struct cfg80211_cqm_config *cqm_config;
struct sk_buff *msg;
s32 rssi_level;

cfg80211_cqm_rssi_update(rdev, dev);
wdev_lock(wdev);
cqm_config = rcu_dereference_protected(wdev->cqm_config,
lockdep_is_held(&wdev->mtx));
if (!wdev->cqm_config)
goto unlock;

if (rssi_level == 0)
rssi_level = wdev->cqm_config->last_rssi_event_value;
}
cfg80211_cqm_rssi_update(rdev, wdev->netdev, cqm_config);

msg = cfg80211_prepare_cqm(dev, NULL, gfp);
rssi_level = cqm_config->last_rssi_event_value;
rssi_event = cqm_config->last_rssi_event_type;

msg = cfg80211_prepare_cqm(wdev->netdev, NULL, GFP_KERNEL);
if (!msg)
return;
goto unlock;

if (nla_put_u32(msg, NL80211_ATTR_CQM_RSSI_THRESHOLD_EVENT,
rssi_event))
Expand All @@ -19123,14 +19151,15 @@ void cfg80211_cqm_rssi_notify(struct net_device *dev,
rssi_level))
goto nla_put_failure;

cfg80211_send_cqm(msg, gfp);
cfg80211_send_cqm(msg, GFP_KERNEL);

return;
goto unlock;

nla_put_failure:
nlmsg_free(msg);
unlock:
wdev_unlock(wdev);
}
EXPORT_SYMBOL(cfg80211_cqm_rssi_notify);

void cfg80211_cqm_txe_notify(struct net_device *dev,
const u8 *peer, u32 num_packets,
Expand Down

0 comments on commit 37c20b2

Please sign in to comment.