Oh, wait, there's another bug here ... One that makes more sense.
On Wed, 2023-08-16 at 15:32 +0200, Johannes Berg wrote:
>
> @@ -12629,6 +12630,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;
> @@ -12646,10 +12648,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);
This used to NULL out the value on freeing
> - 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);
> @@ -12666,9 +12664,9 @@ static int nl80211_set_cqm_rssi(struct genl_info *info,
> n_thresholds = 0;
>
> wdev_lock(wdev);
> + old = rcu_dereference_protected(wdev->cqm_config,
> + lockdep_is_held(&wdev->mtx));
> if (n_thresholds) {
> - struct cfg80211_cqm_config *cqm_config;
> -
> cqm_config = kzalloc(struct_size(cqm_config, rssi_thresholds,
> n_thresholds),
> GFP_KERNEL);
> @@ -12683,10 +12681,16 @@ 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);
> }
>
> - 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);
But I didn't put that here! So you obviously have UAF when removing a
CQM config.
johannes
