diff mbox series

Managing debugfs entries and https://syzkaller.appspot.com/bug?extid=d5dc2801166df6d34774

Message ID 1ff58acb-4171-46ff-8a33-821600a8d8e4@yandex.ru (mailing list archive)
State RFC
Delegated to: Johannes Berg
Headers show
Series Managing debugfs entries and https://syzkaller.appspot.com/bug?extid=d5dc2801166df6d34774 | expand

Commit Message

Dmitry Antipov July 18, 2024, 5:03 p.m. UTC 
The following quirk looks like a (briefly tested with CONFIG_KMEMLEAK)
fix for https://syzkaller.appspot.com/bug?extid=d5dc2801166df6d34774:


Dmitry

Comments

Berg, Benjamin July 19, 2024, 9:38 a.m. UTC | #1 
Hi,

hmm, doubt that would work. Fundamentally, the problem is that when we
switch between MLO and non-MLO (multi-link operation), we need to
recreate the debugfs because the layout changes. However, in that case
we should not have any active stations and running
ieee80211_debugfs_recreate_netdev should usually not be problematic.

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.

To fix the error, one could possibly prevent the stations debugfs
entries from being deleted by ieee80211_debugfs_recreate_netdev or also
recreate them. However, keeping them is not really correct unless MLO
is not toggled and I am not sure how straight forward it would be to
recreate them.

Benjamin

On Thu, 2024-07-18 at 20:03 +0300, Dmitry Antipov wrote:
> The following quirk looks like a (briefly tested with CONFIG_KMEMLEAK)
> fix for https://syzkaller.appspot.com/bug?extid=d5dc2801166df6d34774:
> 
> diff --git a/net/mac80211/debugfs_sta.c b/net/mac80211/debugfs_sta.c
> index 1e9389c49a57..8224257e5d93 100644
> --- a/net/mac80211/debugfs_sta.c
> +++ b/net/mac80211/debugfs_sta.c
> @@ -1284,7 +1284,9 @@ void ieee80211_sta_debugfs_add(struct sta_info *sta)
> 
>   void ieee80211_sta_debugfs_remove(struct sta_info *sta)
>   {
> -       debugfs_remove_recursive(sta->debugfs_dir);
> +       if (!sta->debugfs_shared)
> +               debugfs_remove_recursive(sta->debugfs_dir);
> +       sta->debugfs_shared = false;
>          sta->debugfs_dir = NULL;
>   }
> 
> @@ -1319,6 +1321,7 @@ void ieee80211_link_sta_debugfs_add(struct link_sta_info *link_sta)
>                          return;
> 
>                  link_sta->debugfs_dir = link_sta->sta->debugfs_dir;
> +               link_sta->sta->debugfs_shared = true;
>          }
> 
>          DEBUGFS_ADD(ht_capa);
> diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
> index 9195d5a2de0a..d76ba36ca794 100644
> --- a/net/mac80211/sta_info.h
> +++ b/net/mac80211/sta_info.h
> @@ -708,6 +708,7 @@ struct sta_info {
> 
>   #ifdef CONFIG_MAC80211_DEBUGFS
>          struct dentry *debugfs_dir;
> +       bool debugfs_shared;
>   #endif
> 
>          struct codel_params cparams;
> 
> So what about managing debugfs entries wih krefs? E.g.:
> 
> diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
> index 9195d5a2de0a..1f4561533530 100644
> --- a/net/mac80211/sta_info.h
> +++ b/net/mac80211/sta_info.h
> @@ -466,6 +466,15 @@ struct ieee80211_fragment_cache {
>   	unsigned int next;
>   };
> 
> +#ifdef CONFIG_MAC80211_DEBUGFS
> +
> +struct sta_debugfs_entry {
> +	struct dentry *debugfs_dir;
> +	struct kref kref;
> +};
> +
> +#endif /* CONFIG_MAC80211_DEBUGFS */
> +
>   /*
>    * The bandwidth threshold below which the per-station CoDel parameters will be
>    * scaled to be more lenient (to prevent starvation of slow stations). This
> @@ -563,7 +572,7 @@ struct link_sta_info {
>   	enum ieee80211_sta_rx_bandwidth cur_max_bandwidth;
> 
>   #ifdef CONFIG_MAC80211_DEBUGFS
> -	struct dentry *debugfs_dir;
> +	struct sta_debugfs_entry *debugfs_entry;
>   #endif
> 
>   	struct ieee80211_link_sta *pub;
> @@ -707,7 +716,7 @@ struct sta_info {
>   	struct sta_ampdu_mlme ampdu_mlme;
> 
>   #ifdef CONFIG_MAC80211_DEBUGFS
> -	struct dentry *debugfs_dir;
> +	struct sta_debugfs_entry *debugfs_entry;
>   #endif
> 
>   	struct codel_params cparams;
> 
> Dmitry

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
Dmitry Antipov July 23, 2024, 11:19 a.m. UTC | #2 
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;
+
  #ifdef CONFIG_MAC80211_DEBUGFS
  	struct {
  		struct dentry *subdir_stations;
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index b4ad66af3af3..d8e6e411d754 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -215,6 +215,9 @@ static int ieee80211_can_powered_addr_change(struct ieee80211_sub_if_data *sdata
  	if (netif_carrier_ok(sdata->dev))
  		return -EBUSY;

+	if (sdata->sta_count)
+		return -EBUSY;
+
  	/* First check no ROC work is happening on this iface */
  	list_for_each_entry(roc, &local->roc_list, list) {
  		if (roc->sdata != sdata)
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index aa22f09e6d14..42657afb6d22 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -443,6 +443,7 @@ void sta_info_free(struct ieee80211_local *local, struct sta_info *sta)
  #endif

  	sta_info_free_link(&sta->deflink);
+	sta->sdata->sta_count--;
  	kfree(sta);
  }

@@ -691,6 +692,7 @@ __sta_info_alloc(struct ieee80211_sub_if_data *sdata,
  	sta->cparams.ce_threshold_mask = 0;

  	sta_dbg(sdata, "Allocated STA %pM\n", sta->sta.addr);
+	sdata->sta_count++;

  	return sta;

Dmitry
Johannes Berg July 23, 2024, 11:35 a.m. UTC | #3 
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,
Dmitry Antipov July 24, 2024, 12:29 p.m. UTC | #4 
On 7/23/24 2:35 PM, Johannes Berg wrote:

> So more like below.

Thanks.

BTW please also look at https://lore.kernel.org/linux-hardening/202407161121.C00AC44@keescook/T/#t.

Dmitry
diff mbox series

Patch

diff --git a/net/mac80211/debugfs_sta.c b/net/mac80211/debugfs_sta.c
index 1e9389c49a57..8224257e5d93 100644
--- a/net/mac80211/debugfs_sta.c
+++ b/net/mac80211/debugfs_sta.c
@@ -1284,7 +1284,9 @@  void ieee80211_sta_debugfs_add(struct sta_info *sta)

  void ieee80211_sta_debugfs_remove(struct sta_info *sta)
  {
-       debugfs_remove_recursive(sta->debugfs_dir);
+       if (!sta->debugfs_shared)
+               debugfs_remove_recursive(sta->debugfs_dir);
+       sta->debugfs_shared = false;
         sta->debugfs_dir = NULL;
  }

@@ -1319,6 +1321,7 @@  void ieee80211_link_sta_debugfs_add(struct link_sta_info *link_sta)
                         return;

                 link_sta->debugfs_dir = link_sta->sta->debugfs_dir;
+               link_sta->sta->debugfs_shared = true;
         }

         DEBUGFS_ADD(ht_capa);
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 9195d5a2de0a..d76ba36ca794 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -708,6 +708,7 @@  struct sta_info {

  #ifdef CONFIG_MAC80211_DEBUGFS
         struct dentry *debugfs_dir;
+       bool debugfs_shared;
  #endif

         struct codel_params cparams;

So what about managing debugfs entries wih krefs? E.g.:

diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 9195d5a2de0a..1f4561533530 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -466,6 +466,15 @@  struct ieee80211_fragment_cache {
  	unsigned int next;
  };

+#ifdef CONFIG_MAC80211_DEBUGFS
+
+struct sta_debugfs_entry {
+	struct dentry *debugfs_dir;
+	struct kref kref;
+};
+
+#endif /* CONFIG_MAC80211_DEBUGFS */
+
  /*
   * The bandwidth threshold below which the per-station CoDel parameters will be
   * scaled to be more lenient (to prevent starvation of slow stations). This
@@ -563,7 +572,7 @@  struct link_sta_info {
  	enum ieee80211_sta_rx_bandwidth cur_max_bandwidth;

  #ifdef CONFIG_MAC80211_DEBUGFS
-	struct dentry *debugfs_dir;
+	struct sta_debugfs_entry *debugfs_entry;
  #endif

  	struct ieee80211_link_sta *pub;
@@ -707,7 +716,7 @@  struct sta_info {
  	struct sta_ampdu_mlme ampdu_mlme;

  #ifdef CONFIG_MAC80211_DEBUGFS
-	struct dentry *debugfs_dir;
+	struct sta_debugfs_entry *debugfs_entry;
  #endif

  	struct codel_params cparams;