[lvc-project] Managing debugfs entries and https://syzkaller.appspot.com/bug?extid=d5dc2801166df6d34774

[Johannes Berg]

On Tue, 2024-07-23 at 14:19 +0300, Dmitry Antipov wrote:
> On 7/19/24 12:38 PM, Berg, Benjamin wrote:
> 
> > So, the simple way to prevent this error is to make sure that
> > ieee80211_debugfs_recreate_netdev is never called while we have a
> > station. In the case of this report we seem to be getting there via a
> > mac address change (i.e. ieee80211_change_mac) and the sane thing would
> > be to just return -EBUSY instead of permitting the operation to
> > continue.
> 
> Just to check whether I understand this:
> 
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index a3485e4c6132..d5adbe5b3e51 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -1173,6 +1173,8 @@ struct ieee80211_sub_if_data {
> 
>   	u16 restart_active_links;
> 
> +	u32 sta_count;

That's probably one way of doing it, but it's rather ad-hoc, really what
we should be doing is check more things in whether we allow the change
or not, it looks like now it can only happen in the window between
starting auth/assoc and actually having a connection, which is anyway
wrong to allow it in.

So more like below.

johannes


--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -204,7 +204,6 @@ static int ieee80211_can_powered_addr_change(struct ieee80211_sub_if_data *sdata
 	struct ieee80211_roc_work *roc;
 	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_sub_if_data *scan_sdata;
-	int ret = 0;
 
 	lockdep_assert_wiphy(local->hw.wiphy);
 
@@ -220,10 +219,8 @@ static int ieee80211_can_powered_addr_change(struct ieee80211_sub_if_data *sdata
 		if (roc->sdata != sdata)
 			continue;
 
-		if (roc->started) {
-			ret = -EBUSY;
-			goto unlock;
-		}
+		if (roc->started)
+			return -EBUSY;
 	}
 
 	/* And if this iface is scanning */
@@ -231,7 +228,7 @@ static int ieee80211_can_powered_addr_change(struct ieee80211_sub_if_data *sdata
 		scan_sdata = rcu_dereference_protected(local->scan_sdata,
 						       lockdep_is_held(&local->hw.wiphy->mtx));
 		if (sdata == scan_sdata)
-			ret = -EBUSY;
+			return -EBUSY;
 	}
 
 	switch (sdata->vif.type) {
@@ -240,13 +237,15 @@ static int ieee80211_can_powered_addr_change(struct ieee80211_sub_if_data *sdata
 		/* More interface types could be added here but changing the
 		 * address while powered makes the most sense in client modes.
 		 */
+		if (sdata->u.mgd.auth_data || sdata->u.mgd.assoc_data ||
+		    sdata->u.mgd.associated)
+			return -EBUSY;
 		break;
 	default:
-		ret = -EOPNOTSUPP;
+		return -EOPNOTSUPP;
 	}
 
-unlock:
-	return ret;
+	return 0;
 }
 
 static int _ieee80211_change_mac(struct ieee80211_sub_if_data *sdata,