Skip to contentSkip to conversation

staging: rtl8192u: change get_key functions to return 0 instead of -1

PATCH In review Rebecca Mckeever 3 participants 4 messages Apr 20, 2022 Staging

lore
Share & tools
lore

Rebecca Mckeever

author
#0 lore raw

PATCH 1/1 staging: rtl8192u: change get_key functions to return 0 instead of -1

Currently, these three get_key functions return -1 when the provided len value is less a specific key length value, which can result in buffer overflow depending on how the returned value is used. These functions are used in three places in ieee80211/ieee80211_wx.c:

ieee80211_wx_get_encode() :
The behavior of this function will be unchanged.

ieee80211_wx_get_encode_ext() :
The result of the get_key function is written to ext->key_len, resulting in a buffer overflow if the result is negative.

ieee80211_wx_set_encode() :
The behavior of this function will change. When len is less than the key length value, it will set a default key of all 0.

Suggested-by Dan Carpenter <[email protected]>
Signed-off-by Rebecca Mckeever <[email protected]>
---
 drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c | 2 +-
 drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c | 2 +-
 drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c +1 -1
index 101c28265e91..f17d07dad56d 100644
@@ -362,7 +362,7 @@ static int ieee80211_ccmp_get_key(void *key, int len, u8 *seq, void *priv)
 	struct ieee80211_ccmp_data *data = priv;
 
 	if (len < CCMP_TK_LEN)
-		return -1;
+		return 0;
 
 	if (!data->key_set)
 		return 0;
drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c +1 -1
index 689d8843f538..7b120b8cb982 100644
@@ -637,7 +637,7 @@ static int ieee80211_tkip_get_key(void *key, int len, u8 *seq, void *priv)
 	struct ieee80211_tkip_data *tkey = priv;
 
 	if (len < TKIP_KEY_LEN)
-		return -1;
+		return 0;
 
 	if (!tkey->key_set)
 		return 0;
drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c +1 -1
index 8a51ea1dd6e5..a2cdf3bfd1a4 100644
@@ -201,7 +201,7 @@ static int prism2_wep_get_key(void *key, int len, u8 *seq, void *priv)
 	struct prism2_wep_data *wep = priv;
 
 	if (len < wep->key_len)
-		return -1;
+		return 0;
 
 	memcpy(key, wep->key, wep->key_len);
Signature · 1 line
2.32.0
#1 lore raw
Quoted 16 lines · On mercoledì 20 aprile 2022 14:23:28 CEST Rebecca Mckeever wrote:

Currently, these three get_key functions return -1 when the provided len value is less a specific key length value, which can result in buffer overflow depending on how the returned value is used. These functions are used in three places in ieee80211/ieee80211_wx.c:

ieee80211_wx_get_encode() :
The behavior of this function will be unchanged.

ieee80211_wx_get_encode_ext() :
The result of the get_key function is written to ext->key_len, resulting in a buffer overflow if the result is negative.

ieee80211_wx_set_encode() :
The behavior of this function will change. When len is less than the key length value, it will set a default key of all 0.

Quoted 7 lines
Suggested-by Dan Carpenter <[email protected]>
Signed-off-by Rebecca Mckeever <[email protected]>
---
 drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c | 2 +-
 drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c | 2 +-
 drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

I was not able to find the message where Dan suggested this solution. However it looks that we actually have problems in those callers when they get '-1' as the return code.

I didn't look at the code with much attention but I think that returning '0' to signal errors is not so good.

If I'm not missing something from the context, I'd rather return '-EINVAL' and change the callers to check for this specific error that would signal we have "len less than a specific len value". If so, act in accordance to the returned -EINVAL.

For example, in a caller you may test:

if (ext->key_len == -EINVAL || <some other test, if required>)
<error path>
<success path>

Just my thoughts, but, as I said, I haven't checked carefully neither the callers nor the called functions. Maybe that returning '0' is the better solution, but it usually means that we have no errors.

However, I'm 100% sure that, if Dan suggested this specific solution, there must be good reasons behind. If so, your changes are good :)

Thanks,

Fabio M. De Francesco

Review

Quoted 2 lines
(quoted diff) renamed from drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c +0 -0

drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c

drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c static int ieee80211_ccmp_get_key(void *key, int len,

Quoted 4 lines

index 101c28265e91..f17d07dad56d 100644

drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c +0 -0
@@ -362,7 +362,7 @@ static int ieee80211_ccmp_get_key(void *key, int len,

u8 *seq, void *priv)

Review

Quoted 9 lines
drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c +1 -1
 	struct ieee80211_ccmp_data *data = priv;
 
 	if (len < CCMP_TK_LEN)
-		return -1;
+		return 0;
 
 	if (!data->key_set)
 		return 0;
(quoted diff) renamed from drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c +0 -0

drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c

drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c static int ieee80211_tkip_get_key(void *key, int len,

Quoted 4 lines

index 689d8843f538..7b120b8cb982 100644

drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c +0 -0
@@ -637,7 +637,7 @@ static int ieee80211_tkip_get_key(void *key, int len,

u8 *seq, void *priv)

Review

Quoted 9 lines
drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c +1 -1
 	struct ieee80211_tkip_data *tkey = priv;
 
 	if (len < TKIP_KEY_LEN)
-		return -1;
+		return 0;
 
 	if (!tkey->key_set)
 		return 0;
(quoted diff) renamed from drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c +0 -0

drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c

drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c static int prism2_wep_get_key(void *key, int len, u8

Quoted 4 lines

index 8a51ea1dd6e5..a2cdf3bfd1a4 100644

drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c +0 -0
@@ -201,7 +201,7 @@ static int prism2_wep_get_key(void *key, int len, u8

*seq, void *priv)

Quoted 13 lines
drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c +1 -1
 	struct prism2_wep_data *wep = priv;
 
 	if (len < wep->key_len)
-		return -1;
+		return 0;
 
 	memcpy(key, wep->key, wep->key_len);
Signature · 1 line
2.32.0
#2 lore raw
Quoted 37 lines · Fabio M. De Francesco
Quoted 16 lines · On mercoledì 20 aprile 2022 14:23:28 CEST Rebecca Mckeever wrote:

Currently, these three get_key functions return -1 when the provided len value is less a specific key length value, which can result in buffer overflow depending on how the returned value is used. These functions are used in three places in ieee80211/ieee80211_wx.c:

ieee80211_wx_get_encode() :
The behavior of this function will be unchanged.

ieee80211_wx_get_encode_ext() :
The result of the get_key function is written to ext->key_len, resulting in a buffer overflow if the result is negative.

ieee80211_wx_set_encode() :
The behavior of this function will change. When len is less than the key length value, it will set a default key of all 0.

Quoted 7 lines
Suggested-by Dan Carpenter <[email protected]>
Signed-off-by Rebecca Mckeever <[email protected]>
---
 drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c | 2 +-
 drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c | 2 +-
 drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

I was not able to find the message where Dan suggested this solution. However it looks that we actually have problems in those callers when they get '-1' as the return code.

I didn't look at the code with much attention but I think that returning '0' to signal errors is not so good.

If I'm not missing something from the context, I'd rather return '-EINVAL' and change the callers to check for this specific error that would signal we have "len less than a specific len value". If so, act in accordance to the returned -EINVAL.

Quoted 5 lines

For example, in a caller you may test:

if (ext->key_len == -EINVAL || <some other test, if required>)
<error path>
<success path>

These are valid questions.

Ideally the errors wouldn't happen. Maybe they don't happen?

The error handling is copied from ieee80211_wx_get_encode_ext(). An all zero password (rot13 security) is better than plaintext. So this is a conservative fix which potentially fixes some memory corruption and doesn't break anything.

A more extensive fix would require someone who understands the code better or can test it.

regards,
dan carpenter

#3 lore raw
In reply to Rebecca Mckeever
Quoted 12 lines · Rebecca Mckeever

Currently, these three get_key functions return -1 when the provided len value is less a specific key length value, which can result in buffer overflow depending on how the returned value is used. These functions are used in three places in ieee80211/ieee80211_wx.c:

ieee80211_wx_get_encode() :
The behavior of this function will be unchanged.

ieee80211_wx_get_encode_ext() :
The result of the get_key function is written to ext->key_len, resulting in a buffer overflow if the result is negative.

Quoted 6 lines

ieee80211_wx_set_encode() :
The behavior of this function will change. When len is less than the key length value, it will set a default key of all 0.

Suggested-by Dan Carpenter <[email protected]>
Signed-off-by Rebecca Mckeever <[email protected]>

Of course I suggested this one, but reviewing it again it still seems like the right thing. Good commit message. It explains the controversial bit nicely which is the behavior change in ieee80211_wx_set_encode(). When you explain the all controversial bits in advance then it builds trust.

regards,
dan carpenter