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 <[email protected]>
---
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)
qize wang <[email protected]> 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 <[email protected]>
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
A bit late, but a few readability and maintainability thoughts:
On Fri, Nov 29, 2019 at 2:12 AM qize wang <[email protected]> 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: <[email protected]>
tag here. I'm not sure if "just have GregKH on CC" is the right process...
> Signed-off-by: qize wang <[email protected]>
> ---
> 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
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 <[email protected]> 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: <[email protected]>
>
> tag here. I'm not sure if "just have GregKH on CC" is the right process...
Not at all :)
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/[email protected]/T/#u
https://patchwork.kernel.org/patch/11277011/
Regards,
Brian