[PATCH] mac80211: fix NULL pointer dereference in ieee80211_key_alloc()

[PATCH] mac80211: fix NULL pointer dereference in ieee80211_key_alloc()

[Petr Štetiar]

The ieee80211_key struct can be kfree()d several times in the function, for
example if some of the key setup functions fails beforehand, but there's no
check if the struct is still valid before we call memcpy() and INIT_LIST_HEAD()
on it.  In some cases (like it was in my case), if there's missing aes-generic
module it could lead to the following kernel OOPS:

	Unable to handle kernel NULL pointer dereference at virtual address 0000018c
	....
	PC is at memcpy+0x80/0x29c
	...
	Backtrace:
	[<bf11c5e4>] (ieee80211_key_alloc+0x0/0x234 [mac80211]) from [<bf1148b4>] (ieee80211_add_key+0x70/0x12c [mac80211])
	[<bf114844>] (ieee80211_add_key+0x0/0x12c [mac80211]) from [<bf070cc0>] (__cfg80211_set_encryption+0x2a8/0x464 [cfg80211])

Signed-off-by: Petr Štetiar <ynezz@true.cz>
CC: Johannes Berg <johannes@sipsolutions.net>
---
 net/mac80211/key.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index 8c02469..8e49850 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -364,8 +364,11 @@ struct ieee80211_key *ieee80211_key_alloc(u32 cipher, int idx, size_t key_len,
 		}
 		break;
 	}
-	memcpy(key->conf.key, key_data, key_len);
-	INIT_LIST_HEAD(&key->list);
+
+	if (!IS_ERR(key)) {
+		memcpy(key->conf.key, key_data, key_len);
+		INIT_LIST_HEAD(&key->list);
+	}
 
 	return key;
 }
-- 
1.7.1

Re: [PATCH] mac80211: fix NULL pointer dereference in ieee80211_key_alloc()

[Johannes Berg]

Hi Petr,

On Sat, 2011-03-26 at 18:32 +0100, Petr Štetiar wrote:
> The ieee80211_key struct can be kfree()d several times in the function, for
> example if some of the key setup functions fails beforehand, but there's no
> check if the struct is still valid before we call memcpy() and INIT_LIST_HEAD()
> on it.  In some cases (like it was in my case), if there's missing aes-generic
> module it could lead to the following kernel OOPS:
> 
> 	Unable to handle kernel NULL pointer dereference at virtual address 0000018c
> 	....
> 	PC is at memcpy+0x80/0x29c
> 	...
> 	Backtrace:
> 	[<bf11c5e4>] (ieee80211_key_alloc+0x0/0x234 [mac80211]) from [<bf1148b4>] (ieee80211_add_key+0x70/0x12c [mac80211])
> 	[<bf114844>] (ieee80211_add_key+0x0/0x12c [mac80211]) from [<bf070cc0>] (__cfg80211_set_encryption+0x2a8/0x464 [cfg80211])

Thanks for the patch, good find.

> @@ -364,8 +364,11 @@ struct ieee80211_key *ieee80211_key_alloc(u32 cipher, int idx, size_t key_len,
>  		}
>  		break;
>  	}
> -	memcpy(key->conf.key, key_data, key_len);
> -	INIT_LIST_HEAD(&key->list);
> +
> +	if (!IS_ERR(key)) {
> +		memcpy(key->conf.key, key_data, key_len);
> +		INIT_LIST_HEAD(&key->list);
> +	}

I think instead we should change the two IS_ERR(tfm) places to:

	if (IS_ERR(key->u.xxx.tfm)) {
		err = PTR_ERR(...);
		kfree(key);
		return ERR_PTR(err);
	}

Do you want to submit a patch for that, or would you prefer me to handle
it?

johannes

[PATCH v2] mac80211: fix NULL pointer dereference in ieee80211_key_alloc()

[Petr Štetiar]

The ieee80211_key struct can be kfree()d several times in the function, for
example if some of the key setup functions fails beforehand, but there's no
check if the struct is still valid before we call memcpy() and INIT_LIST_HEAD()
on it.  In some cases (like it was in my case), if there's missing aes-generic
module it could lead to the following kernel OOPS:

	Unable to handle kernel NULL pointer dereference at virtual address 0000018c
	....
	PC is at memcpy+0x80/0x29c
	...
	Backtrace:
	[<bf11c5e4>] (ieee80211_key_alloc+0x0/0x234 [mac80211]) from [<bf1148b4>] (ieee80211_add_key+0x70/0x12c [mac80211])
	[<bf114844>] (ieee80211_add_key+0x0/0x12c [mac80211]) from [<bf070cc0>] (__cfg80211_set_encryption+0x2a8/0x464 [cfg80211])

Signed-off-by: Petr Štetiar <ynezz@true.cz>
CC: Johannes Berg <johannes@sipsolutions.net>
---
 net/mac80211/key.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index 8c02469..09cf1f2 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -342,7 +342,7 @@ struct ieee80211_key *ieee80211_key_alloc(u32 cipher, int idx, size_t key_len,
 		if (IS_ERR(key->u.ccmp.tfm)) {
 			err = PTR_ERR(key->u.ccmp.tfm);
 			kfree(key);
-			key = ERR_PTR(err);
+			return ERR_PTR(err);
 		}
 		break;
 	case WLAN_CIPHER_SUITE_AES_CMAC:
@@ -360,7 +360,7 @@ struct ieee80211_key *ieee80211_key_alloc(u32 cipher, int idx, size_t key_len,
 		if (IS_ERR(key->u.aes_cmac.tfm)) {
 			err = PTR_ERR(key->u.aes_cmac.tfm);
 			kfree(key);
-			key = ERR_PTR(err);
+			return ERR_PTR(err);
 		}
 		break;
 	}
-- 
1.7.1

Re: [PATCH v2] mac80211: fix NULL pointer dereference in ieee80211_key_alloc()

[Johannes Berg]

On Sun, 2011-03-27 at 13:31 +0200, Petr Štetiar wrote:
> The ieee80211_key struct can be kfree()d several times in the function, for
> example if some of the key setup functions fails beforehand, but there's no
> check if the struct is still valid before we call memcpy() and INIT_LIST_HEAD()
> on it.  In some cases (like it was in my case), if there's missing aes-generic
> module it could lead to the following kernel OOPS:
> 
> 	Unable to handle kernel NULL pointer dereference at virtual address 0000018c
> 	....
> 	PC is at memcpy+0x80/0x29c
> 	...
> 	Backtrace:
> 	[<bf11c5e4>] (ieee80211_key_alloc+0x0/0x234 [mac80211]) from [<bf1148b4>] (ieee80211_add_key+0x70/0x12c [mac80211])
> 	[<bf114844>] (ieee80211_add_key+0x0/0x12c [mac80211]) from [<bf070cc0>] (__cfg80211_set_encryption+0x2a8/0x464 [cfg80211])
> 
> Signed-off-by: Petr Štetiar <ynezz@true.cz>
> CC: Johannes Berg <johannes@sipsolutions.net>

Thanks!

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

> ---
>  net/mac80211/key.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mac80211/key.c b/net/mac80211/key.c
> index 8c02469..09cf1f2 100644
> --- a/net/mac80211/key.c
> +++ b/net/mac80211/key.c
> @@ -342,7 +342,7 @@ struct ieee80211_key *ieee80211_key_alloc(u32 cipher, int idx, size_t key_len,
>  		if (IS_ERR(key->u.ccmp.tfm)) {
>  			err = PTR_ERR(key->u.ccmp.tfm);
>  			kfree(key);
> -			key = ERR_PTR(err);
> +			return ERR_PTR(err);
>  		}
>  		break;
>  	case WLAN_CIPHER_SUITE_AES_CMAC:
> @@ -360,7 +360,7 @@ struct ieee80211_key *ieee80211_key_alloc(u32 cipher, int idx, size_t key_len,
>  		if (IS_ERR(key->u.aes_cmac.tfm)) {
>  			err = PTR_ERR(key->u.aes_cmac.tfm);
>  			kfree(key);
> -			key = ERR_PTR(err);
> +			return ERR_PTR(err);
>  		}
>  		break;
>  	}