| From f5b3c75a1a0b1d874bca7ae48d1b158b27f71db4 Mon Sep 17 00:00:00 2001 |
| From: Brian Norris <briannorris@chromium.org> |
| Date: Fri, 6 Dec 2019 11:45:35 -0800 |
| Subject: [PATCH] mwifiex: drop most magic numbers from |
| mwifiex_process_tdls_action_frame() |
| |
| commit 70e5b8f445fd27fde0c5583460e82539a7242424 upstream. |
| |
| Before commit 1e58252e334d ("mwifiex: Fix heap overflow in |
| mmwifiex_process_tdls_action_frame()"), |
| mwifiex_process_tdls_action_frame() already had too many magic numbers. |
| But this commit just added a ton more, in the name of checking for |
| buffer overflows. That seems like a really bad idea. |
| |
| Let's make these magic numbers a little less magic, by |
| (a) factoring out 'pos[1]' as 'ie_len' |
| (b) using 'sizeof' on the appropriate source or destination fields where |
| possible, instead of bare numbers |
| (c) dropping redundant checks, per below. |
| |
| Regarding redundant checks: the beginning of the loop has this: |
| |
| if (pos + 2 + pos[1] > end) |
| break; |
| |
| but then individual 'case's include stuff like this: |
| |
| if (pos > end - 3) |
| return; |
| if (pos[1] != 1) |
| return; |
| |
| Note that the second 'return' (validating the length, pos[1]) combined |
| with the above condition (ensuring 'pos + 2 + length' doesn't exceed |
| 'end'), makes the first 'return' (whose 'if' can be reworded as 'pos > |
| end - pos[1] - 2') redundant. Rather than unwind the magic numbers |
| there, just drop those conditions. |
| |
| Fixes: 1e58252e334d ("mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame()") |
| Signed-off-by: Brian Norris <briannorris@chromium.org> |
| Signed-off-by: Kalle Valo <kvalo@codeaurora.org> |
| Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> |
| |
| diff --git a/drivers/net/wireless/marvell/mwifiex/tdls.c b/drivers/net/wireless/marvell/mwifiex/tdls.c |
| index 6058c48d56dc..b6b7bbe168eb 100644 |
| --- a/drivers/net/wireless/marvell/mwifiex/tdls.c |
| +++ b/drivers/net/wireless/marvell/mwifiex/tdls.c |
| @@ -897,7 +897,7 @@ void mwifiex_process_tdls_action_frame(struct mwifiex_private *priv, |
| u8 *peer, *pos, *end; |
| u8 i, action, basic; |
| u16 cap = 0; |
| - int ie_len = 0; |
| + int ies_len = 0; |
| |
| if (len < (sizeof(struct ethhdr) + 3)) |
| return; |
| @@ -919,7 +919,7 @@ void mwifiex_process_tdls_action_frame(struct mwifiex_private *priv, |
| pos = buf + sizeof(struct ethhdr) + 4; |
| /* payload 1+ category 1 + action 1 + dialog 1 */ |
| cap = get_unaligned_le16(pos); |
| - ie_len = len - sizeof(struct ethhdr) - TDLS_REQ_FIX_LEN; |
| + ies_len = len - sizeof(struct ethhdr) - TDLS_REQ_FIX_LEN; |
| pos += 2; |
| break; |
| |
| @@ -929,7 +929,7 @@ void mwifiex_process_tdls_action_frame(struct mwifiex_private *priv, |
| /* payload 1+ category 1 + action 1 + dialog 1 + status code 2*/ |
| pos = buf + sizeof(struct ethhdr) + 6; |
| cap = get_unaligned_le16(pos); |
| - ie_len = len - sizeof(struct ethhdr) - TDLS_RESP_FIX_LEN; |
| + ies_len = len - sizeof(struct ethhdr) - TDLS_RESP_FIX_LEN; |
| pos += 2; |
| break; |
| |
| @@ -937,7 +937,7 @@ void mwifiex_process_tdls_action_frame(struct mwifiex_private *priv, |
| if (len < (sizeof(struct ethhdr) + TDLS_CONFIRM_FIX_LEN)) |
| return; |
| pos = buf + sizeof(struct ethhdr) + TDLS_CONFIRM_FIX_LEN; |
| - ie_len = len - sizeof(struct ethhdr) - TDLS_CONFIRM_FIX_LEN; |
| + ies_len = len - sizeof(struct ethhdr) - TDLS_CONFIRM_FIX_LEN; |
| break; |
| default: |
| mwifiex_dbg(priv->adapter, ERROR, "Unknown TDLS frame type.\n"); |
| @@ -950,33 +950,33 @@ void mwifiex_process_tdls_action_frame(struct mwifiex_private *priv, |
| |
| sta_ptr->tdls_cap.capab = cpu_to_le16(cap); |
| |
| - for (end = pos + ie_len; pos + 1 < end; pos += 2 + pos[1]) { |
| - if (pos + 2 + pos[1] > end) |
| + for (end = pos + ies_len; pos + 1 < end; pos += 2 + pos[1]) { |
| + u8 ie_len = pos[1]; |
| + |
| + if (pos + 2 + ie_len > end) |
| break; |
| |
| switch (*pos) { |
| case WLAN_EID_SUPP_RATES: |
| - if (pos[1] > 32) |
| + if (ie_len > sizeof(sta_ptr->tdls_cap.rates)) |
| return; |
| - sta_ptr->tdls_cap.rates_len = pos[1]; |
| - for (i = 0; i < pos[1]; i++) |
| + sta_ptr->tdls_cap.rates_len = ie_len; |
| + for (i = 0; i < ie_len; i++) |
| sta_ptr->tdls_cap.rates[i] = pos[i + 2]; |
| break; |
| |
| case WLAN_EID_EXT_SUPP_RATES: |
| - if (pos[1] > 32) |
| + if (ie_len > sizeof(sta_ptr->tdls_cap.rates)) |
| return; |
| basic = sta_ptr->tdls_cap.rates_len; |
| - if (pos[1] > 32 - basic) |
| + if (ie_len > sizeof(sta_ptr->tdls_cap.rates) - basic) |
| return; |
| - for (i = 0; i < pos[1]; i++) |
| + for (i = 0; i < ie_len; i++) |
| sta_ptr->tdls_cap.rates[basic + i] = pos[i + 2]; |
| - sta_ptr->tdls_cap.rates_len += pos[1]; |
| + sta_ptr->tdls_cap.rates_len += ie_len; |
| break; |
| case WLAN_EID_HT_CAPABILITY: |
| - if (pos > end - sizeof(struct ieee80211_ht_cap) - 2) |
| - return; |
| - if (pos[1] != sizeof(struct ieee80211_ht_cap)) |
| + if (ie_len != sizeof(struct ieee80211_ht_cap)) |
| return; |
| /* copy the ie's value into ht_capb*/ |
| memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos + 2, |
| @@ -984,59 +984,45 @@ void mwifiex_process_tdls_action_frame(struct mwifiex_private *priv, |
| sta_ptr->is_11n_enabled = 1; |
| break; |
| case WLAN_EID_HT_OPERATION: |
| - if (pos > end - |
| - sizeof(struct ieee80211_ht_operation) - 2) |
| - return; |
| - if (pos[1] != sizeof(struct ieee80211_ht_operation)) |
| + if (ie_len != 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) |
| + if (ie_len != sizeof(pos[2])) |
| 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)) |
| + if (ie_len < sizeof(struct ieee_types_header)) |
| return; |
| - if (pos[1] > 8) |
| + if (ie_len > 8) |
| return; |
| memcpy((u8 *)&sta_ptr->tdls_cap.extcap, pos, |
| sizeof(struct ieee_types_header) + |
| - min_t(u8, pos[1], 8)); |
| + min_t(u8, ie_len, 8)); |
| break; |
| case WLAN_EID_RSN: |
| - if (pos > end - sizeof(struct ieee_types_header)) |
| + if (ie_len < sizeof(struct ieee_types_header)) |
| return; |
| - if (pos[1] < sizeof(struct ieee_types_header)) |
| - return; |
| - if (pos[1] > IEEE_MAX_IE_SIZE - |
| + if (ie_len > 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 - |
| + min_t(u8, ie_len, IEEE_MAX_IE_SIZE - |
| sizeof(struct ieee_types_header))); |
| break; |
| case WLAN_EID_QOS_CAPA: |
| - if (pos > end - 3) |
| - return; |
| - if (pos[1] != 1) |
| + if (ie_len != sizeof(pos[2])) |
| return; |
| sta_ptr->tdls_cap.qos_info = pos[2]; |
| break; |
| case WLAN_EID_VHT_OPERATION: |
| if (priv->adapter->is_hw_11ac_capable) { |
| - if (pos > end - |
| - sizeof(struct ieee80211_vht_operation) - 2) |
| - return; |
| - if (pos[1] != |
| + if (ie_len != |
| sizeof(struct ieee80211_vht_operation)) |
| return; |
| /* copy the ie's value into vhtoper*/ |
| @@ -1046,10 +1032,7 @@ void mwifiex_process_tdls_action_frame(struct mwifiex_private *priv, |
| break; |
| case WLAN_EID_VHT_CAPABILITY: |
| if (priv->adapter->is_hw_11ac_capable) { |
| - if (pos > end - |
| - sizeof(struct ieee80211_vht_cap) - 2) |
| - return; |
| - if (pos[1] != sizeof(struct ieee80211_vht_cap)) |
| + if (ie_len != sizeof(struct ieee80211_vht_cap)) |
| return; |
| /* copy the ie's value into vhtcap*/ |
| memcpy((u8 *)&sta_ptr->tdls_cap.vhtcap, pos + 2, |
| @@ -1059,9 +1042,7 @@ void mwifiex_process_tdls_action_frame(struct mwifiex_private *priv, |
| break; |
| case WLAN_EID_AID: |
| if (priv->adapter->is_hw_11ac_capable) { |
| - if (pos > end - 4) |
| - return; |
| - if (pos[1] != 2) |
| + if (ie_len != sizeof(u16)) |
| return; |
| sta_ptr->tdls_cap.aid = |
| get_unaligned_le16((pos + 2)); |
| -- |
| 2.7.4 |
| |