[PATCH] mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame()

[PATCH] mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame()

[qize wang]

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>
---
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 09313047beed..7caf1d26124a 100644
--- a/drivers/net/wireless/marvell/mwifiex/tdls.c
+++ b/drivers/net/wireless/marvell/mwifiex/tdls.c
@@ -953,59 +953,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;
		}
-- 
2.14.3 (Apple Git-98)

Re: [PATCH] mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame()

[Dan Carpenter]

On Fri, Nov 22, 2019 at 05:43:49PM +0800, qize wang wrote:
> 		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,
                                                                 ^^^^^^^

I don't understand why we changed "pos" to "pos + 2".  Presumably there
is a reason, but it needs to explained in the commit message.


> 			       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;

I was confused by the "- 2" as well in the earlier versions of this
patch but Marvell approved it so I assumed it was correct.  It would be
nice if this patch were tested.

regards,
dan carpenter

RE: [EXT] Re: [PATCH] mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame()

[Ganapathi Bhat]

Hi Dan,

> > +			/* copy the ie's value into ht_capb*/
> > +			memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos + 2,
>                                                                  ^^^^^^^
> 
> I don't understand why we changed "pos" to "pos + 2".  Presumably there is
> a reason, but it needs to explained in the commit message.

I think, we were doing wrong in the original code. We are supposed to use 'pos + 2' itself, instead of just 'pos'. This is because, 'pos' is pointing to 'ieee_types_header', followed by the actual data and the destination do not start with (i.e. it do not contain) 'ieee_types_header'(ex: 'sta_ptr->tdls_cap.ht_oper').

Also, there are few places were the destination starts with 'ieee_types_header'(ex: 'sta_ptr->tdls_cap.extcap'), which need just 'pos'.

Regards,
Ganapathi

Re: [EXT] Re: [PATCH] mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame()

[Dan Carpenter]

On Fri, Nov 22, 2019 at 11:40:40AM +0000, Ganapathi Bhat wrote:
> Hi Dan,
> 
> > > +			/* copy the ie's value into ht_capb*/
> > > +			memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos + 2,
> >                                                                  ^^^^^^^
> > 
> > I don't understand why we changed "pos" to "pos + 2".  Presumably there is
> > a reason, but it needs to explained in the commit message.
> 
> I think, we were doing wrong in the original code. We are supposed to use 'pos + 2' itself, instead of just 'pos'. This is because, 'pos' is pointing to 'ieee_types_header', followed by the actual data and the destination do not start with (i.e. it do not contain) 'ieee_types_header'(ex: 'sta_ptr->tdls_cap.ht_oper').
> 
> Also, there are few places were the destination starts with 'ieee_types_header'(ex: 'sta_ptr->tdls_cap.extcap'), which need just 'pos'.

I assumed it was something like this but it needs to be explained in
the commit message.

regards,
dan carpenter

Re: [EXT] Re: [PATCH] mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame()

[qize wang]

Hi, dan

I forget to explain  the reason of changing "pos" to "pos + 2” in the commit message.
Thank you for your suggestion.
IE is TLV struct, but ht_cap and  ht_oper aren’t TLV struct,the origin marvell driver code is wrong. 
we fix the bug by changing pos(the address of IE) to  pos+2 ( the address of IE’s value ).

regards,
qize wang

> 在 2019年11月22日，下午8:37，Dan Carpenter <dan.carpenter@oracle.com> 写道：
> 
> On Fri, Nov 22, 2019 at 11:40:40AM +0000, Ganapathi Bhat wrote:
>> Hi Dan,
>> 
>>>> +			/* copy the ie's value into ht_capb*/
>>>> +			memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos + 2,
>>>                                                                 ^^^^^^^
>>> 
>>> I don't understand why we changed "pos" to "pos + 2".  Presumably there is
>>> a reason, but it needs to explained in the commit message.
>> 
>> I think, we were doing wrong in the original code. We are supposed to use 'pos + 2' itself, instead of just 'pos'. This is because, 'pos' is pointing to 'ieee_types_header', followed by the actual data and the destination do not start with (i.e. it do not contain) 'ieee_types_header'(ex: 'sta_ptr->tdls_cap.ht_oper').
>> 
>> Also, there are few places were the destination starts with 'ieee_types_header'(ex: 'sta_ptr->tdls_cap.extcap'), which need just 'pos'.
> 
> I assumed it was something like this but it needs to be explained in
> the commit message.
> 
> regards,
> dan carpenter
> 

Re: [EXT] [PATCH] mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame()

[qize wang]

This fix addresses CVE-2019-14901

> 在 2019年11月22日，下午10:27，qize wang <wangqize888888888@gmail.com> 写道：
> 
> Hi, dan
> 
> I forget to explain  the reason of changing "pos" to "pos + 2” in the commit message.
> Thank you for your suggestion.
> IE is TLV struct, but ht_cap and  ht_oper aren’t TLV struct,the origin marvell driver code is wrong. 
> we fix the bug by changing pos(the address of IE) to  pos+2 ( the address of IE’s value ).
> 
> regards,
> qize wang
> 
>> 在 2019年11月22日，下午8:37，Dan Carpenter <dan.carpenter@oracle.com> 写道：
>> 
>> On Fri, Nov 22, 2019 at 11:40:40AM +0000, Ganapathi Bhat wrote:
>>> Hi Dan,
>>> 
>>>>> +			/* copy the ie's value into ht_capb*/
>>>>> +			memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos + 2,
>>>>                                                                ^^^^^^^
>>>> 
>>>> I don't understand why we changed "pos" to "pos + 2".  Presumably there is
>>>> a reason, but it needs to explained in the commit message.
>>> 
>>> I think, we were doing wrong in the original code. We are supposed to use 'pos + 2' itself, instead of just 'pos'. This is because, 'pos' is pointing to 'ieee_types_header', followed by the actual data and the destination do not start with (i.e. it do not contain) 'ieee_types_header'(ex: 'sta_ptr->tdls_cap.ht_oper').
>>> 
>>> Also, there are few places were the destination starts with 'ieee_types_header'(ex: 'sta_ptr->tdls_cap.extcap'), which need just 'pos'.
>> 
>> I assumed it was something like this but it needs to be explained in
>> the commit message.
>> 
>> regards,
>> dan carpenter
>> 
> 

Re: [PATCH] mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame()

[Kalle Valo]

qize wang <wangqize888888888@gmail.com> wrote:

> 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>

Please fix the commit log as explained by Dan.

Patch set to Changes Requested.

-- 
https://patchwork.kernel.org/patch/11257535/

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

Re: [PATCH] mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame()

[Kalle Valo]

(adding back linux-wireless)

Hi qize,

qize wang <wangqize888888888@gmail.com> writes:

> sorry, I am a new guy.

Welcome to the Linux community!

But most important thing first: please don't send email privately,
instead post any questions you have to the list. Normally I don't reply
to private email (I get way too much mail), but making an exception this
time as you are new.

> I have some question: Should I need to submit a new version patch to
> linux-wireless( like [patch v2]....) or reply a comment in the old
> patch path. (https://patchwork.kernel.org/patch/11257535/) ?

Yes, please submit v2 and include a change log what you changed. More
info in the link below.

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