Skip to contentSkip to conversation

mwifiex: Prevent memory corruption handling keys

PATCH In review Dan Carpenter 3 participants 3 messages Jul 8, 2020 – Jul 15, 2020

Subsystems (2)
Kernel janitors Marvell mwifiex wireless driver
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 mwifiex: Prevent memory corruption handling keys

The length of the key comes from the network and it's a 16 bit number. It needs to be capped to prevent a buffer overflow.

Fixes 5e6e3a92b9a4 ("wireless: mwifiex: initial commit for Marvell mwifiex driver")
Signed-off-by Dan Carpenter <[email protected]>

---
This is from static analysis. Please review it carefully.

 .../wireless/marvell/mwifiex/sta_cmdresp.c    | 22 +++++++++++++------
 1 file changed, 15 insertions(+), 7 deletions(-)
drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c +15 -6
index f21660149f58..267b02cde30c 100644
@@ -580,6 +580,11 @@ static int mwifiex_ret_802_11_key_material_v1(struct mwifiex_private *priv,
 {
 	struct host_cmd_ds_802_11_key_material *key  						&resp->params.key_material;
+	int len;
+
+	len = le16_to_cpu(key->key_param_set.key_len);
+	if (len > sizeof(key->key_param_set.key))
+		return -EINVAL;
 
 	if (le16_to_cpu(key->action) = HostCmd_ACT_GEN_SET) {
 		if ((le16_to_cpu(key->key_param_set.key_info) & KEY_MCAST)) {

@@ -593,9 +598,8 @@ static int mwifiex_ret_802_11_key_material_v1(struct mwifiex_private *priv,
 
 	memset(priv->aes_key.key_param_set.key, 0,
 	       sizeof(key->key_param_set.key));
-	priv->aes_key.key_param_set.key_len = key->key_param_set.key_len;
-	memcpy(priv->aes_key.key_param_set.key, key->key_param_set.key,
-	       le16_to_cpu(priv->aes_key.key_param_set.key_len));
+	priv->aes_key.key_param_set.key_len = cpu_to_le16(len);
+	memcpy(priv->aes_key.key_param_set.key, key->key_param_set.key, len);
 
 	return 0;
 }

@@ -610,9 +614,14 @@ static int mwifiex_ret_802_11_key_material_v2(struct mwifiex_private *priv,
 					      struct host_cmd_ds_command *resp)
 {
 	struct host_cmd_ds_802_11_key_material_v2 *key_v2;
-	__le16 len;
+	int len;
 
 	key_v2 = &resp->params.key_material_v2;
+
+	len = le16_to_cpu(key_v2->key_param_set.key_params.aes.key_len);
+	if (len > WLAN_KEY_LEN_CCMP)
+		return -EINVAL;
+
 	if (le16_to_cpu(key_v2->action) = HostCmd_ACT_GEN_SET) {
 		if ((le16_to_cpu(key_v2->key_param_set.key_info) & KEY_MCAST)) {
 			mwifiex_dbg(priv->adapter, INFO, "info: key: GTK is set\n");

@@ -628,10 +637,9 @@ static int mwifiex_ret_802_11_key_material_v2(struct mwifiex_private *priv,
 	memset(priv->aes_key_v2.key_param_set.key_params.aes.key, 0,
 	       WLAN_KEY_LEN_CCMP);
 	priv->aes_key_v2.key_param_set.key_params.aes.key_len -				key_v2->key_param_set.key_params.aes.key_len;
-	len = priv->aes_key_v2.key_param_set.key_params.aes.key_len;
+				cpu_to_le16(len);
 	memcpy(priv->aes_key_v2.key_param_set.key_params.aes.key,
-	       key_v2->key_param_set.key_params.aes.key, le16_to_cpu(len));
+	       key_v2->key_param_set.key_params.aes.key, len);
 
 	return 0;
 }
Signature · 1 line
2.27.0
Acked-by on v1 1/1
#1 lore raw

Hi Dan,

Quoted 2 lines

The length of the key comes from the network and it's a 16 bit number. It needs to be capped to prevent a buffer overflow.

Thank you for this change.

Acked-by Ganapathi Bhat <[email protected]>

Regards,
Ganapathi

#2 lore raw
In reply to Dan Carpenter
Quoted 6 lines · Dan Carpenter

The length of the key comes from the network and it's a 16 bit number. It needs to be capped to prevent a buffer overflow.

Fixes 5e6e3a92b9a4 ("wireless: mwifiex: initial commit for Marvell mwifiex driver")
Signed-off-by Dan Carpenter <[email protected]>
Acked-by Ganapathi Bhat <[email protected]>

Patch applied to wireless-drivers-next.git, thanks.

e18696786548 mwifiex: Prevent memory corruption handling keys

Signature · 3 lines
https://patchwork.kernel.org/patch/11651443/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches