[PATCH] mac80211: Fix debugfs union misuse and pointer corruption

[PATCH] mac80211: Fix debugfs union misuse and pointer corruption

[Jouni Malinen]

debugfs union in struct ieee80211_sub_if_data is misused by including a
common default_key dentry as a union member. This ends occupying the same
memory area with the first dentry in other union members (structures;
usually drop_unencrypted). Consequently, debugfs operations on
default_key symlinks and drop_unencrypted entry are using the same
dentry pointer even though they are supposed to be separate ones. This
can lead to removing entries incorrectly or potentially leaving
something behind since one of the dentry pointers gets lost.

Fix this by moving the default_key dentry to a new struct
(common_debugfs) that contains dentries (more to be added in future)
that are shared by all vif types. The debugfs union must only be used
for vif type-specific entries to avoid this type of pointer corruption.

Signed-off-by: Jouni Malinen <jouni.malinen@atheros.com>


Index: wireless-testing/net/mac80211/ieee80211_i.h
===================================================================
--- wireless-testing.orig/net/mac80211/ieee80211_i.h
+++ wireless-testing/net/mac80211/ieee80211_i.h
@@ -498,8 +498,10 @@ struct ieee80211_sub_if_data {
 		struct {
 			struct dentry *mode;
 		} monitor;
-		struct dentry *default_key;
 	} debugfs;
+	struct {
+		struct dentry *default_key;
+	} common_debugfs;
 
 #ifdef CONFIG_MAC80211_MESH
 	struct dentry *mesh_stats_dir;
Index: wireless-testing/net/mac80211/debugfs_key.c
===================================================================
--- wireless-testing.orig/net/mac80211/debugfs_key.c
+++ wireless-testing/net/mac80211/debugfs_key.c
@@ -265,7 +265,7 @@ void ieee80211_debugfs_key_add_default(s
 	key = sdata->default_key;
 	if (key) {
 		sprintf(buf, "../keys/%d", key->debugfs.cnt);
-		sdata->debugfs.default_key =
+		sdata->common_debugfs.default_key =
 			debugfs_create_symlink("default_key",
 					       sdata->debugfsdir, buf);
 	} else
@@ -277,8 +277,8 @@ void ieee80211_debugfs_key_remove_defaul
 	if (!sdata)
 		return;
 
-	debugfs_remove(sdata->debugfs.default_key);
-	sdata->debugfs.default_key = NULL;
+	debugfs_remove(sdata->common_debugfs.default_key);
+	sdata->common_debugfs.default_key = NULL;
 }
 
 void ieee80211_debugfs_key_sta_del(struct ieee80211_key *key,


-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH] mac80211: Fix debugfs union misuse and pointer corruption

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2596 bytes --]

On Thu, 2008-08-28 at 15:12 +0300, Jouni Malinen wrote:
> debugfs union in struct ieee80211_sub_if_data is misused by including a
> common default_key dentry as a union member. This ends occupying the same
> memory area with the first dentry in other union members (structures;
> usually drop_unencrypted). Consequently, debugfs operations on
> default_key symlinks and drop_unencrypted entry are using the same
> dentry pointer even though they are supposed to be separate ones. This
> can lead to removing entries incorrectly or potentially leaving
> something behind since one of the dentry pointers gets lost.
> 
> Fix this by moving the default_key dentry to a new struct
> (common_debugfs) that contains dentries (more to be added in future)
> that are shared by all vif types. The debugfs union must only be used
> for vif type-specific entries to avoid this type of pointer corruption.
> 
> Signed-off-by: Jouni Malinen <jouni.malinen@atheros.com>

Acked-by: Johannes Berg <johannes@sipsolutions.net>

also the other similar patch, sorry about this, it's surely my mistake.

> 
> Index: wireless-testing/net/mac80211/ieee80211_i.h
> ===================================================================
> --- wireless-testing.orig/net/mac80211/ieee80211_i.h
> +++ wireless-testing/net/mac80211/ieee80211_i.h
> @@ -498,8 +498,10 @@ struct ieee80211_sub_if_data {
>  		struct {
>  			struct dentry *mode;
>  		} monitor;
> -		struct dentry *default_key;
>  	} debugfs;
> +	struct {
> +		struct dentry *default_key;
> +	} common_debugfs;
>  
>  #ifdef CONFIG_MAC80211_MESH
>  	struct dentry *mesh_stats_dir;
> Index: wireless-testing/net/mac80211/debugfs_key.c
> ===================================================================
> --- wireless-testing.orig/net/mac80211/debugfs_key.c
> +++ wireless-testing/net/mac80211/debugfs_key.c
> @@ -265,7 +265,7 @@ void ieee80211_debugfs_key_add_default(s
>  	key = sdata->default_key;
>  	if (key) {
>  		sprintf(buf, "../keys/%d", key->debugfs.cnt);
> -		sdata->debugfs.default_key =
> +		sdata->common_debugfs.default_key =
>  			debugfs_create_symlink("default_key",
>  					       sdata->debugfsdir, buf);
>  	} else
> @@ -277,8 +277,8 @@ void ieee80211_debugfs_key_remove_defaul
>  	if (!sdata)
>  		return;
>  
> -	debugfs_remove(sdata->debugfs.default_key);
> -	sdata->debugfs.default_key = NULL;
> +	debugfs_remove(sdata->common_debugfs.default_key);
> +	sdata->common_debugfs.default_key = NULL;
>  }
>  
>  void ieee80211_debugfs_key_sta_del(struct ieee80211_key *key,
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] mac80211: Fix debugfs union misuse and pointer corruption

[Jouni Malinen]

On Thu, Aug 28, 2008 at 02:18:43PM +0200, Johannes Berg wrote:

> Acked-by: Johannes Berg <johannes@sipsolutions.net>
> 
> also the other similar patch, sorry about this, it's surely my mistake.

Dave already took the other fix. This new one is a bit less likely to
cause real problems for most users (i.e., I'm not too concerned about
getting this into 2.6.27).

I'm trying to fix the netdev:mon.wlan0 not being removed correctly
with my 802.11w patches (incorrect order in debugfs keys vs. netdev
deletion) and was seeing very strange behavior here.. I had just added
yet another dentry into the union since I was just duplicating the code
used for default_key symlink.. None of the debug prints made any sense
until I noticed that these "two" pointers were actually changing values
at the same time.. Unions are evil if you do not notice you are using
one ;-).

-- 
Jouni Malinen                                            PGP id EFC895FA