diff mbox series

mwifiex: drop most magic numbers from mwifiex_process_tdls_action_frame()

Message ID 20191206194535.150179-1-briannorris@chromium.org (mailing list archive)
State Accepted
Commit 70e5b8f445fd27fde0c5583460e82539a7242424
Delegated to: Kalle Valo
Headers show
Series mwifiex: drop most magic numbers from mwifiex_process_tdls_action_frame() | expand

Commit Message

Brian Norris Dec. 6, 2019, 7:45 p.m. UTC 
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>
---
AFAICT, the existing commit (1e58252e334d) isn't wrong, per se -- just
very poorly styled -- so this probably doesn't need to go to -stable.
Not sure if it's a candidate for wireless-drivers (where the original
commit currently sites) vs. wireless-drivers-next.

 drivers/net/wireless/marvell/mwifiex/tdls.c | 75 ++++++++-------------
 1 file changed, 28 insertions(+), 47 deletions(-)

Comments

Kalle Valo Dec. 9, 2019, 10:58 a.m. UTC | #1 
Brian Norris <briannorris@chromium.org> writes:

> AFAICT, the existing commit (1e58252e334d) isn't wrong, per se -- just
> very poorly styled -- so this probably doesn't need to go to -stable.
> Not sure if it's a candidate for wireless-drivers (where the original
> commit currently sites) vs. wireless-drivers-next.

I'll try to do so that I'll put this patch to "Awaiting Upstream" state
and apply it to w-d-next once 1e58252e334d is merged to w-d-next. Feel
free to remind me then that happens :)
Brian Norris Jan. 13, 2020, 7:05 p.m. UTC | #2 
On Mon, Dec 9, 2019 at 2:58 AM Kalle Valo <kvalo@codeaurora.org> wrote:
>
> Brian Norris <briannorris@chromium.org> writes:
>
> > AFAICT, the existing commit (1e58252e334d) isn't wrong, per se -- just
> > very poorly styled -- so this probably doesn't need to go to -stable.
> > Not sure if it's a candidate for wireless-drivers (where the original
> > commit currently sites) vs. wireless-drivers-next.
>
> I'll try to do so that I'll put this patch to "Awaiting Upstream" state
> and apply it to w-d-next once 1e58252e334d is merged to w-d-next. Feel
> free to remind me then that happens :)

It's that time!

Brian
Kalle Valo Jan. 14, 2020, 1:41 p.m. UTC | #3 
Brian Norris <briannorris@chromium.org> writes:

> On Mon, Dec 9, 2019 at 2:58 AM Kalle Valo <kvalo@codeaurora.org> wrote:
>>
>> Brian Norris <briannorris@chromium.org> writes:
>>
>> > AFAICT, the existing commit (1e58252e334d) isn't wrong, per se -- just
>> > very poorly styled -- so this probably doesn't need to go to -stable.
>> > Not sure if it's a candidate for wireless-drivers (where the original
>> > commit currently sites) vs. wireless-drivers-next.
>>
>> I'll try to do so that I'll put this patch to "Awaiting Upstream" state
>> and apply it to w-d-next once 1e58252e334d is merged to w-d-next. Feel
>> free to remind me then that happens :)
>
> It's that time!

Thanks, changed the state to 'Under Review' which means this is back in
my queue.
Kalle Valo Jan. 26, 2020, 11:37 a.m. UTC | #4 
Brian Norris <briannorris@chromium.org> wrote:

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

Patch applied to wireless-drivers-next.git, thanks.

70e5b8f445fd mwifiex: drop most magic numbers from mwifiex_process_tdls_action_frame()
diff mbox series

Patch

diff --git a/drivers/net/wireless/marvell/mwifiex/tdls.c b/drivers/net/wireless/marvell/mwifiex/tdls.c
index 7caf1d26124a..f8f282ce39bd 100644
--- a/drivers/net/wireless/marvell/mwifiex/tdls.c
+++ b/drivers/net/wireless/marvell/mwifiex/tdls.c
@@ -894,7 +894,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;
@@ -916,7 +916,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;
 
@@ -926,7 +926,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;
 
@@ -934,7 +934,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");
@@ -947,33 +947,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,
@@ -981,59 +981,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*/
@@ -1043,10 +1029,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,
@@ -1056,9 +1039,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));