Re: [RFC PATCH v3 6.1] wifi: cfg80211: fix cqm_config access race

[Johannes Berg <johannes@sipsolutions.net>]

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