Linux-Wireless Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3] mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame()
@ 2019-11-29 10:10 qize wang
  2019-12-02 14:54 ` Kalle Valo
  2019-12-02 23:16 ` Brian Norris
  0 siblings, 2 replies; 5+ messages in thread
From: qize wang @ 2019-11-29 10:10 UTC (permalink / raw)
  To: linux-wireless
  Cc: amitkarwar, nishants, gbhat, huxinming820, kvalo, greg,
	dan.carpenter, solar, wangqize888888888

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.

IE is TLV struct, but ht_cap and  ht_oper aren’t TLV struct.
the origin marvell driver code is wrong:

memcpy(&sta_ptr->tdls_cap.ht_oper, pos,....
memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos,...

Fix the bug by changing pos(the address of IE) to
pos+2 ( the address of IE value ).

v3: change commit log

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)


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame()
  2019-11-29 10:10 [PATCH v3] mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame() qize wang
@ 2019-12-02 14:54 ` Kalle Valo
  2019-12-02 23:16 ` Brian Norris
  1 sibling, 0 replies; 5+ messages in thread
From: Kalle Valo @ 2019-12-02 14:54 UTC (permalink / raw)
  To: qize wang
  Cc: linux-wireless, amitkarwar, nishants, gbhat, huxinming820, kvalo,
	greg, dan.carpenter, solar

qize wang <wangqize888888888@gmail.com> writes:

> 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.
>
> IE is TLV struct, but ht_cap and  ht_oper aren’t TLV struct.
> the origin marvell driver code is wrong:
>
> memcpy(&sta_ptr->tdls_cap.ht_oper, pos,....
> memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos,...
>
> Fix the bug by changing pos(the address of IE) to
> pos+2 ( the address of IE value ).
>
> v3: change commit log
>
> Signed-off-by: qize wang <wangqize888888888@gmail.com>

Applied manually (removed the changelog from commit), thanks.

1e58252e334d mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame()

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame()
  2019-11-29 10:10 [PATCH v3] mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame() qize wang
  2019-12-02 14:54 ` Kalle Valo
@ 2019-12-02 23:16 ` Brian Norris
  2019-12-03 12:14   ` Greg KH
  2019-12-06 19:49   ` Brian Norris
  1 sibling, 2 replies; 5+ messages in thread
From: Brian Norris @ 2019-12-02 23:16 UTC (permalink / raw)
  To: qize wang
  Cc: linux-wireless, amit karwar, Nishant Sarmukadam, Ganapathi Bhat,
	Xinming Hu, Kalle Valo, greg, Dan Carpenter, Solar Designer

A bit late, but a few readability and maintainability thoughts:

On Fri, Nov 29, 2019 at 2:12 AM 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.
>
> IE is TLV struct, but ht_cap and  ht_oper aren’t TLV struct.
> the origin marvell driver code is wrong:
>
> memcpy(&sta_ptr->tdls_cap.ht_oper, pos,....
> memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos,...
>
> Fix the bug by changing pos(the address of IE) to
> pos+2 ( the address of IE value ).
>
> v3: change commit log
>

Would have been great to have a

Cc: <stable@vger.kernel.org>

tag here. I'm not sure if "just have GregKH on CC" is the right process...

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

Really, you needed a magic '32' here? Would be much clearer with:

  if (pos[1] > sizeof(sta_ptr->tdls_cap.rates))

Same with many of the other cases.

> +                               return;

Is 'return' really the right answer for all of these? Just because,
e.g., the rates IE is larger than our internal struct, should we
really drop the entire frame? Should we be continuing to parse the
other IEs, if possible? Or is this overflow a sign of a totally
damaged (possibly malicious) frame, because it's required to be
smaller than this? (Sorry, I didn't read the spec here yet.)

>                         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_HT_CAPABILITY:
> -                       memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos,
> +                       if (pos > end - sizeof(struct ieee80211_ht_cap) - 2)
> +                               return;

For checks like this ("past 'end'"), it does make sense to return.

Brian

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame()
  2019-12-02 23:16 ` Brian Norris
@ 2019-12-03 12:14   ` Greg KH
  2019-12-06 19:49   ` Brian Norris
  1 sibling, 0 replies; 5+ messages in thread
From: Greg KH @ 2019-12-03 12:14 UTC (permalink / raw)
  To: Brian Norris
  Cc: qize wang, linux-wireless, amit karwar, Nishant Sarmukadam,
	Ganapathi Bhat, Xinming Hu, Kalle Valo, Dan Carpenter,
	Solar Designer

On Mon, Dec 02, 2019 at 03:16:35PM -0800, Brian Norris wrote:
> A bit late, but a few readability and maintainability thoughts:
> 
> On Fri, Nov 29, 2019 at 2:12 AM 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.
> >
> > IE is TLV struct, but ht_cap and  ht_oper aren’t TLV struct.
> > the origin marvell driver code is wrong:
> >
> > memcpy(&sta_ptr->tdls_cap.ht_oper, pos,....
> > memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos,...
> >
> > Fix the bug by changing pos(the address of IE) to
> > pos+2 ( the address of IE value ).
> >
> > v3: change commit log
> >
> 
> Would have been great to have a
> 
> Cc: <stable@vger.kernel.org>
> 
> tag here. I'm not sure if "just have GregKH on CC" is the right process...

Not at all :)


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame()
  2019-12-02 23:16 ` Brian Norris
  2019-12-03 12:14   ` Greg KH
@ 2019-12-06 19:49   ` Brian Norris
  1 sibling, 0 replies; 5+ messages in thread
From: Brian Norris @ 2019-12-06 19:49 UTC (permalink / raw)
  To: qize wang
  Cc: linux-wireless, amit karwar, Nishant Sarmukadam, Ganapathi Bhat,
	Xinming Hu, Kalle Valo, greg, Dan Carpenter, Solar Designer

On Mon, Dec 02, 2019 at 03:16:35PM -0800, Brian Norris wrote:
> A bit late, but a few readability and maintainability thoughts:

I worded most of my suggestions in the form of a patch here:

[PATCH] mwifiex: drop most magic numbers from mwifiex_process_tdls_action_frame()
https://lore.kernel.org/linux-wireless/20191206194535.150179-1-briannorris@chromium.org/T/#u
https://patchwork.kernel.org/patch/11277011/

Regards,
Brian

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-29 10:10 [PATCH v3] mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame() qize wang
2019-12-02 14:54 ` Kalle Valo
2019-12-02 23:16 ` Brian Norris
2019-12-03 12:14   ` Greg KH
2019-12-06 19:49   ` Brian Norris

Linux-Wireless Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-wireless/0 linux-wireless/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-wireless linux-wireless/ https://lore.kernel.org/linux-wireless \
		linux-wireless@vger.kernel.org
	public-inbox-index linux-wireless

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-wireless


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git