[E/D/B,3/3] UBUNTU: SAUCE: mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame()
diff mbox series

Message ID 20191126083917.5995-4-stefan.bader@canonical.com
State New
Headers show
Series
  • Untitled series #145033
Related show

Commit Message

Stefan Bader Nov. 26, 2019, 8:39 a.m. UTC
From: qize wang <wangqize888888888@gmail.com>

mwifiex_process_tdls_action_frame() without checking
the incoming tdls infomation element's vality before use it,
this may cause multi heap buffer overflows.

Fix them by putting vality check before use it.

Signed-off-by: qize wang <wangqize888888888@gmail.com>

CVE-2019-14901

(cherry picked from https://patchwork.kernel.org/patch/11257535/)
Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
---
 drivers/net/wireless/marvell/mwifiex/tdls.c | 70 +++++++++++++++++++--
 1 file changed, 64 insertions(+), 6 deletions(-)

Comments

Kleber Souza Nov. 27, 2019, 4:21 p.m. UTC | #1
On 26.11.19 09:39, Stefan Bader wrote:
> From: qize wang <wangqize888888888@gmail.com>
> 
> mwifiex_process_tdls_action_frame() without checking
> the incoming tdls infomation element's vality before use it,
> this may cause multi heap buffer overflows.
> 
> Fix them by putting vality check before use it.
> 
> Signed-off-by: qize wang <wangqize888888888@gmail.com>
> 
> CVE-2019-14901
> 
> (cherry picked from https://patchwork.kernel.org/patch/11257535/)
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  drivers/net/wireless/marvell/mwifiex/tdls.c | 70 +++++++++++++++++++--
>  1 file changed, 64 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/tdls.c b/drivers/net/wireless/marvell/mwifiex/tdls.c
> index 18e654dc34c6..7f60214852c0 100644
> --- a/drivers/net/wireless/marvell/mwifiex/tdls.c
> +++ b/drivers/net/wireless/marvell/mwifiex/tdls.c
> @@ -954,59 +954,117 @@ void mwifiex_process_tdls_action_frame(struct mwifiex_private *priv,
>  
>  		switch (*pos) {
>  		case WLAN_EID_SUPP_RATES:
> +			if (pos[1] > 32)
> +				return;
>  			sta_ptr->tdls_cap.rates_len = pos[1];
>  			for (i = 0; i < pos[1]; i++)
>  				sta_ptr->tdls_cap.rates[i] = pos[i + 2];
>  			break;
>  
>  		case WLAN_EID_EXT_SUPP_RATES:
> +			if (pos[1] > 32)
> +				return;
>  			basic = sta_ptr->tdls_cap.rates_len;
> +			if (pos[1] > 32 - basic)
> +				return;
>  			for (i = 0; i < pos[1]; i++)
>  				sta_ptr->tdls_cap.rates[basic + i] = pos[i + 2];
>  			sta_ptr->tdls_cap.rates_len += pos[1];
>  			break;
>  		case WLAN_EID_HT_CAPABILITY:
> -			memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos,
> +			if (pos > end - sizeof(struct ieee80211_ht_cap) - 2)
> +				return;
> +			if (pos[1] != sizeof(struct ieee80211_ht_cap))
> +				return;
> +			/* copy the ie's value into ht_capb*/
> +			memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos + 2,

This is changing the memcpy to start the copy from 'pos' to 'pos + 2',
but following the discussion on the patchwork link it seems the original
code was wrong and it was fixed here but without a comment on the commit
message. So if accepted upstream the commit message might get changed.

>  			       sizeof(struct ieee80211_ht_cap));
>  			sta_ptr->is_11n_enabled = 1;
>  			break;
>  		case WLAN_EID_HT_OPERATION:
> -			memcpy(&sta_ptr->tdls_cap.ht_oper, pos,
> +			if (pos > end -
> +			    sizeof(struct ieee80211_ht_operation) - 2)
> +				return;
> +			if (pos[1] != sizeof(struct ieee80211_ht_operation))
> +				return;
> +			/* copy the ie's value into ht_oper*/
> +			memcpy(&sta_ptr->tdls_cap.ht_oper, pos + 2,
>  			       sizeof(struct ieee80211_ht_operation));
>  			break;
>  		case WLAN_EID_BSS_COEX_2040:
> +			if (pos > end - 3)
> +				return;
> +			if (pos[1] != 1)
> +				return;
>  			sta_ptr->tdls_cap.coex_2040 = pos[2];
>  			break;
>  		case WLAN_EID_EXT_CAPABILITY:
> +			if (pos > end - sizeof(struct ieee_types_header))
> +				return;
> +			if (pos[1] < sizeof(struct ieee_types_header))
> +				return;
> +			if (pos[1] > 8)
> +				return;
>  			memcpy((u8 *)&sta_ptr->tdls_cap.extcap, pos,
>  			       sizeof(struct ieee_types_header) +
>  			       min_t(u8, pos[1], 8));
>  			break;
>  		case WLAN_EID_RSN:
> +			if (pos > end - sizeof(struct ieee_types_header))
> +				return;
> +			if (pos[1] < sizeof(struct ieee_types_header))
> +				return;
> +			if (pos[1] > IEEE_MAX_IE_SIZE -
> +			    sizeof(struct ieee_types_header))
> +				return;
>  			memcpy((u8 *)&sta_ptr->tdls_cap.rsn_ie, pos,
>  			       sizeof(struct ieee_types_header) +
>  			       min_t(u8, pos[1], IEEE_MAX_IE_SIZE -
>  				     sizeof(struct ieee_types_header)));
>  			break;
>  		case WLAN_EID_QOS_CAPA:
> +			if (pos > end - 3)
> +				return;
> +			if (pos[1] != 1)
> +				return;
>  			sta_ptr->tdls_cap.qos_info = pos[2];
>  			break;
>  		case WLAN_EID_VHT_OPERATION:
> -			if (priv->adapter->is_hw_11ac_capable)
> -				memcpy(&sta_ptr->tdls_cap.vhtoper, pos,
> +			if (priv->adapter->is_hw_11ac_capable) {
> +				if (pos > end -
> +				    sizeof(struct ieee80211_vht_operation) - 2)
> +					return;
> +				if (pos[1] !=
> +				    sizeof(struct ieee80211_vht_operation))
> +					return;
> +				/* copy the ie's value into vhtoper*/
> +				memcpy(&sta_ptr->tdls_cap.vhtoper, pos + 2,
>  				       sizeof(struct ieee80211_vht_operation));
> +			}
>  			break;
>  		case WLAN_EID_VHT_CAPABILITY:
>  			if (priv->adapter->is_hw_11ac_capable) {
> -				memcpy((u8 *)&sta_ptr->tdls_cap.vhtcap, pos,
> +				if (pos > end -
> +				    sizeof(struct ieee80211_vht_cap) - 2)
> +					return;
> +				if (pos[1] != sizeof(struct ieee80211_vht_cap))
> +					return;
> +				/* copy the ie's value into vhtcap*/
> +				memcpy((u8 *)&sta_ptr->tdls_cap.vhtcap, pos + 2,
>  				       sizeof(struct ieee80211_vht_cap));
>  				sta_ptr->is_11ac_enabled = 1;
>  			}
>  			break;
>  		case WLAN_EID_AID:
> -			if (priv->adapter->is_hw_11ac_capable)
> +			if (priv->adapter->is_hw_11ac_capable) {
> +				if (pos > end - 4)
> +					return;
> +				if (pos[1] != 2)
> +					return;
>  				sta_ptr->tdls_cap.aid =
>  					get_unaligned_le16((pos + 2));
> +			}
> +			break;
>  		default:
>  			break;
>  		}
>
Stefan Bader Nov. 27, 2019, 5:38 p.m. UTC | #2
On 27.11.19 17:21, Kleber Souza wrote:
> On 26.11.19 09:39, Stefan Bader wrote:
>> From: qize wang <wangqize888888888@gmail.com>
>>
>> mwifiex_process_tdls_action_frame() without checking
>> the incoming tdls infomation element's vality before use it,
>> this may cause multi heap buffer overflows.
>>
>> Fix them by putting vality check before use it.
>>
>> Signed-off-by: qize wang <wangqize888888888@gmail.com>
>>
>> CVE-2019-14901
>>
>> (cherry picked from https://patchwork.kernel.org/patch/11257535/)
>> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
>> ---
>>  drivers/net/wireless/marvell/mwifiex/tdls.c | 70 +++++++++++++++++++--
>>  1 file changed, 64 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/wireless/marvell/mwifiex/tdls.c b/drivers/net/wireless/marvell/mwifiex/tdls.c
>> index 18e654dc34c6..7f60214852c0 100644
>> --- a/drivers/net/wireless/marvell/mwifiex/tdls.c
>> +++ b/drivers/net/wireless/marvell/mwifiex/tdls.c
>> @@ -954,59 +954,117 @@ void mwifiex_process_tdls_action_frame(struct mwifiex_private *priv,
>>  
>>  		switch (*pos) {
>>  		case WLAN_EID_SUPP_RATES:
>> +			if (pos[1] > 32)
>> +				return;
>>  			sta_ptr->tdls_cap.rates_len = pos[1];
>>  			for (i = 0; i < pos[1]; i++)
>>  				sta_ptr->tdls_cap.rates[i] = pos[i + 2];
>>  			break;
>>  
>>  		case WLAN_EID_EXT_SUPP_RATES:
>> +			if (pos[1] > 32)
>> +				return;
>>  			basic = sta_ptr->tdls_cap.rates_len;
>> +			if (pos[1] > 32 - basic)
>> +				return;
>>  			for (i = 0; i < pos[1]; i++)
>>  				sta_ptr->tdls_cap.rates[basic + i] = pos[i + 2];
>>  			sta_ptr->tdls_cap.rates_len += pos[1];
>>  			break;
>>  		case WLAN_EID_HT_CAPABILITY:
>> -			memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos,
>> +			if (pos > end - sizeof(struct ieee80211_ht_cap) - 2)
>> +				return;
>> +			if (pos[1] != sizeof(struct ieee80211_ht_cap))
>> +				return;
>> +			/* copy the ie's value into ht_capb*/
>> +			memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos + 2,
> 
> This is changing the memcpy to start the copy from 'pos' to 'pos + 2',
> but following the discussion on the patchwork link it seems the original
> code was wrong and it was fixed here but without a comment on the commit
> message. So if accepted upstream the commit message might get changed.

What I understood from the upstream discussion there is two different targets of
memcpy's. Some are of type header which still need the additional 2 bytes. The
others are of a different type which does not contain the header part.

> 
>>  			       sizeof(struct ieee80211_ht_cap));
>>  			sta_ptr->is_11n_enabled = 1;
>>  			break;
>>  		case WLAN_EID_HT_OPERATION:
>> -			memcpy(&sta_ptr->tdls_cap.ht_oper, pos,
>> +			if (pos > end -
>> +			    sizeof(struct ieee80211_ht_operation) - 2)
>> +				return;
>> +			if (pos[1] != sizeof(struct ieee80211_ht_operation))
>> +				return;
>> +			/* copy the ie's value into ht_oper*/
>> +			memcpy(&sta_ptr->tdls_cap.ht_oper, pos + 2,
>>  			       sizeof(struct ieee80211_ht_operation));
>>  			break;
>>  		case WLAN_EID_BSS_COEX_2040:
>> +			if (pos > end - 3)
>> +				return;
>> +			if (pos[1] != 1)
>> +				return;
>>  			sta_ptr->tdls_cap.coex_2040 = pos[2];
>>  			break;
>>  		case WLAN_EID_EXT_CAPABILITY:
>> +			if (pos > end - sizeof(struct ieee_types_header))
>> +				return;
>> +			if (pos[1] < sizeof(struct ieee_types_header))
>> +				return;
>> +			if (pos[1] > 8)
>> +				return;
>>  			memcpy((u8 *)&sta_ptr->tdls_cap.extcap, pos,
>>  			       sizeof(struct ieee_types_header) +
>>  			       min_t(u8, pos[1], 8));
>>  			break;
>>  		case WLAN_EID_RSN:
>> +			if (pos > end - sizeof(struct ieee_types_header))
>> +				return;
>> +			if (pos[1] < sizeof(struct ieee_types_header))
>> +				return;
>> +			if (pos[1] > IEEE_MAX_IE_SIZE -
>> +			    sizeof(struct ieee_types_header))
>> +				return;
>>  			memcpy((u8 *)&sta_ptr->tdls_cap.rsn_ie, pos,
>>  			       sizeof(struct ieee_types_header) +
>>  			       min_t(u8, pos[1], IEEE_MAX_IE_SIZE -
>>  				     sizeof(struct ieee_types_header)));
>>  			break;
>>  		case WLAN_EID_QOS_CAPA:
>> +			if (pos > end - 3)
>> +				return;
>> +			if (pos[1] != 1)
>> +				return;
>>  			sta_ptr->tdls_cap.qos_info = pos[2];
>>  			break;
>>  		case WLAN_EID_VHT_OPERATION:
>> -			if (priv->adapter->is_hw_11ac_capable)
>> -				memcpy(&sta_ptr->tdls_cap.vhtoper, pos,
>> +			if (priv->adapter->is_hw_11ac_capable) {
>> +				if (pos > end -
>> +				    sizeof(struct ieee80211_vht_operation) - 2)
>> +					return;
>> +				if (pos[1] !=
>> +				    sizeof(struct ieee80211_vht_operation))
>> +					return;
>> +				/* copy the ie's value into vhtoper*/
>> +				memcpy(&sta_ptr->tdls_cap.vhtoper, pos + 2,
>>  				       sizeof(struct ieee80211_vht_operation));
>> +			}
>>  			break;
>>  		case WLAN_EID_VHT_CAPABILITY:
>>  			if (priv->adapter->is_hw_11ac_capable) {
>> -				memcpy((u8 *)&sta_ptr->tdls_cap.vhtcap, pos,
>> +				if (pos > end -
>> +				    sizeof(struct ieee80211_vht_cap) - 2)
>> +					return;
>> +				if (pos[1] != sizeof(struct ieee80211_vht_cap))
>> +					return;
>> +				/* copy the ie's value into vhtcap*/
>> +				memcpy((u8 *)&sta_ptr->tdls_cap.vhtcap, pos + 2,
>>  				       sizeof(struct ieee80211_vht_cap));
>>  				sta_ptr->is_11ac_enabled = 1;
>>  			}
>>  			break;
>>  		case WLAN_EID_AID:
>> -			if (priv->adapter->is_hw_11ac_capable)
>> +			if (priv->adapter->is_hw_11ac_capable) {
>> +				if (pos > end - 4)
>> +					return;
>> +				if (pos[1] != 2)
>> +					return;
>>  				sta_ptr->tdls_cap.aid =
>>  					get_unaligned_le16((pos + 2));
>> +			}
>> +			break;
>>  		default:
>>  			break;
>>  		}
>>
>

Patch
diff mbox series

diff --git a/drivers/net/wireless/marvell/mwifiex/tdls.c b/drivers/net/wireless/marvell/mwifiex/tdls.c
index 18e654dc34c6..7f60214852c0 100644
--- a/drivers/net/wireless/marvell/mwifiex/tdls.c
+++ b/drivers/net/wireless/marvell/mwifiex/tdls.c
@@ -954,59 +954,117 @@  void mwifiex_process_tdls_action_frame(struct mwifiex_private *priv,
 
 		switch (*pos) {
 		case WLAN_EID_SUPP_RATES:
+			if (pos[1] > 32)
+				return;
 			sta_ptr->tdls_cap.rates_len = pos[1];
 			for (i = 0; i < pos[1]; i++)
 				sta_ptr->tdls_cap.rates[i] = pos[i + 2];
 			break;
 
 		case WLAN_EID_EXT_SUPP_RATES:
+			if (pos[1] > 32)
+				return;
 			basic = sta_ptr->tdls_cap.rates_len;
+			if (pos[1] > 32 - basic)
+				return;
 			for (i = 0; i < pos[1]; i++)
 				sta_ptr->tdls_cap.rates[basic + i] = pos[i + 2];
 			sta_ptr->tdls_cap.rates_len += pos[1];
 			break;
 		case WLAN_EID_HT_CAPABILITY:
-			memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos,
+			if (pos > end - sizeof(struct ieee80211_ht_cap) - 2)
+				return;
+			if (pos[1] != sizeof(struct ieee80211_ht_cap))
+				return;
+			/* copy the ie's value into ht_capb*/
+			memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos + 2,
 			       sizeof(struct ieee80211_ht_cap));
 			sta_ptr->is_11n_enabled = 1;
 			break;
 		case WLAN_EID_HT_OPERATION:
-			memcpy(&sta_ptr->tdls_cap.ht_oper, pos,
+			if (pos > end -
+			    sizeof(struct ieee80211_ht_operation) - 2)
+				return;
+			if (pos[1] != sizeof(struct ieee80211_ht_operation))
+				return;
+			/* copy the ie's value into ht_oper*/
+			memcpy(&sta_ptr->tdls_cap.ht_oper, pos + 2,
 			       sizeof(struct ieee80211_ht_operation));
 			break;
 		case WLAN_EID_BSS_COEX_2040:
+			if (pos > end - 3)
+				return;
+			if (pos[1] != 1)
+				return;
 			sta_ptr->tdls_cap.coex_2040 = pos[2];
 			break;
 		case WLAN_EID_EXT_CAPABILITY:
+			if (pos > end - sizeof(struct ieee_types_header))
+				return;
+			if (pos[1] < sizeof(struct ieee_types_header))
+				return;
+			if (pos[1] > 8)
+				return;
 			memcpy((u8 *)&sta_ptr->tdls_cap.extcap, pos,
 			       sizeof(struct ieee_types_header) +
 			       min_t(u8, pos[1], 8));
 			break;
 		case WLAN_EID_RSN:
+			if (pos > end - sizeof(struct ieee_types_header))
+				return;
+			if (pos[1] < sizeof(struct ieee_types_header))
+				return;
+			if (pos[1] > IEEE_MAX_IE_SIZE -
+			    sizeof(struct ieee_types_header))
+				return;
 			memcpy((u8 *)&sta_ptr->tdls_cap.rsn_ie, pos,
 			       sizeof(struct ieee_types_header) +
 			       min_t(u8, pos[1], IEEE_MAX_IE_SIZE -
 				     sizeof(struct ieee_types_header)));
 			break;
 		case WLAN_EID_QOS_CAPA:
+			if (pos > end - 3)
+				return;
+			if (pos[1] != 1)
+				return;
 			sta_ptr->tdls_cap.qos_info = pos[2];
 			break;
 		case WLAN_EID_VHT_OPERATION:
-			if (priv->adapter->is_hw_11ac_capable)
-				memcpy(&sta_ptr->tdls_cap.vhtoper, pos,
+			if (priv->adapter->is_hw_11ac_capable) {
+				if (pos > end -
+				    sizeof(struct ieee80211_vht_operation) - 2)
+					return;
+				if (pos[1] !=
+				    sizeof(struct ieee80211_vht_operation))
+					return;
+				/* copy the ie's value into vhtoper*/
+				memcpy(&sta_ptr->tdls_cap.vhtoper, pos + 2,
 				       sizeof(struct ieee80211_vht_operation));
+			}
 			break;
 		case WLAN_EID_VHT_CAPABILITY:
 			if (priv->adapter->is_hw_11ac_capable) {
-				memcpy((u8 *)&sta_ptr->tdls_cap.vhtcap, pos,
+				if (pos > end -
+				    sizeof(struct ieee80211_vht_cap) - 2)
+					return;
+				if (pos[1] != sizeof(struct ieee80211_vht_cap))
+					return;
+				/* copy the ie's value into vhtcap*/
+				memcpy((u8 *)&sta_ptr->tdls_cap.vhtcap, pos + 2,
 				       sizeof(struct ieee80211_vht_cap));
 				sta_ptr->is_11ac_enabled = 1;
 			}
 			break;
 		case WLAN_EID_AID:
-			if (priv->adapter->is_hw_11ac_capable)
+			if (priv->adapter->is_hw_11ac_capable) {
+				if (pos > end - 4)
+					return;
+				if (pos[1] != 2)
+					return;
 				sta_ptr->tdls_cap.aid =
 					get_unaligned_le16((pos + 2));
+			}
+			break;
 		default:
 			break;
 		}