wifi: mwifiex: validate action frame fixed fields

mwifiex_process_mgmt_packet() accepts an rx_pkt_length as small as a four-address struct ieee80211_hdr plus the two-byte firmware length prefix. After stripping the prefix, mwifiex_parse_mgmt_packet() can receive a buffer equal to sizeof(struct ieee80211_hdr).

For action frames, the parser reads the category immediately after that header and, for a public action frame, reads the following action code without verifying that either field is present. A minimal frame therefore reads one or two bytes beyond the RX buffer.

Validate the header and require the category and public action-code fields before reading them. mwifiex parses the firmware four-address layout before removing addr4, so add ETH_ALEN to the standard IEEE80211_MIN_ACTION_SIZE() offsets.

---
Changes in v2:
- Add a local header check and express the action field sizes with IEEE80211_MIN_ACTION_SIZE(), accounting for the firmware layout.

 drivers/net/wireless/marvell/mwifiex/util.c | 9 +++++++++
 1 file changed, 9 insertions(+)

index 7d3631d212236..4043590137391 100644
@@ -313,13 +313,22 @@ mwifiex_parse_mgmt_packet(struct mwifiex_private *priv, u8 *payload, u16 len,
 	u8 category, action_code, *addr2;
 	struct ieee80211_hdr *ieee_hdr = (void *)payload;
 
+	if (len < sizeof(*ieee_hdr))
+		return -1;
+
 	stype = (le16_to_cpu(ieee_hdr->frame_control) & IEEE80211_FCTL_STYPE);
 
 	switch (stype) {
 	case IEEE80211_STYPE_ACTION:
+		if (len < IEEE80211_MIN_ACTION_SIZE(category) + ETH_ALEN)
+			return -1;
+
 		category = *(payload + sizeof(struct ieee80211_hdr));
 		switch (category) {
 		case WLAN_CATEGORY_PUBLIC:
+			if (len < IEEE80211_MIN_ACTION_SIZE(action_code) + ETH_ALEN)
+				return -1;
+
 			action_code = *(payload + sizeof(struct ieee80211_hdr)
 					+ 1);
 			if (action_code == WLAN_PUB_ACTION_TDLS_DISCOVER_RES) {

2.50.1 (Apple Git-155)

I double-checked the arithmetic and, yes, I made a mistake. The header check is redundant while the other two checks are needed.

mwifiex_process_mgmt_packet() already validates:

	if (pkt_len < sizeof(struct ieee80211_hdr) + sizeof(pkt_len)) {

After subtracting sizeof(pkt_len), the len passed into mwifiex_parse_mgmt_packet() is at least sizeof(struct ieee80211_hdr). So the if (len < sizeof(*ieee_hdr)) guard is redundant.

The remaining two guards check that the category and action_code bytes are present beyond the header. Keeping these subtype-specific checks in the parser places them next to the corresponding reads.

I will drop the redundant header check and resend as v3.

Thanks,
Zhao

mwifiex_process_mgmt_packet() accepts an rx_pkt_length as small as a four-address struct ieee80211_hdr plus the two-byte firmware length prefix. After stripping the prefix, mwifiex_parse_mgmt_packet() can receive a buffer equal to sizeof(struct ieee80211_hdr).

For action frames, the parser reads the category byte immediately after that header and, for a public action frame, reads the following action code byte without verifying that either field is present. A minimal frame therefore reads one or two bytes beyond the RX buffer.

Require the category and public action-code fields before reading them. mwifiex parses the firmware four-address layout before removing addr4, so add ETH_ALEN to the standard IEEE80211_MIN_ACTION_SIZE() offsets.

---
Changes in v3:
- Drop the redundant parser-local header check; the caller already guarantees the complete four-address header after removing the two-byte firmware prefix.

Changes in v2:
- Express the action-field sizes with IEEE80211_MIN_ACTION_SIZE(), accounting for the firmware four-address layout.

---
 drivers/net/wireless/marvell/mwifiex/util.c | 6 ++++++
 1 file changed, 6 insertions(+)

index 7d3631d21223..e54a86ecaa33 100644
@@ -317,9 +317,15 @@ mwifiex_parse_mgmt_packet(struct mwifiex_private *priv, u8 *payload, u16 len,
 
 	switch (stype) {
 	case IEEE80211_STYPE_ACTION:
+		if (len < IEEE80211_MIN_ACTION_SIZE(category) + ETH_ALEN)
+			return -1;
+
 		category = *(payload + sizeof(struct ieee80211_hdr));
 		switch (category) {
 		case WLAN_CATEGORY_PUBLIC:
+			if (len < IEEE80211_MIN_ACTION_SIZE(action_code) + ETH_ALEN)
+				return -1;
+
 			action_code = *(payload + sizeof(struct ieee80211_hdr)
 					+ 1);
 			if (action_code == WLAN_PUB_ACTION_TDLS_DISCOVER_RES) {

2.50.1 (Apple Git-155)

Hi,

I think this is another one of those cases where we're working hard to validate against the firmware-reported packet length, but we're not actually checking that the reported length fits within the real skb size (skb->len).

If you want to follow up on that, that could be worth doing in mwifiex_process_mgmt_packet().

mwifiex_process_mgmt_packet() accepts an rx_pkt_length as small as a four-address struct ieee80211_hdr plus the two-byte firmware length prefix. After stripping the prefix, mwifiex_parse_mgmt_packet() can receive a frame equal to sizeof(struct ieee80211_hdr).

For action frames, the parser reads the category byte immediately after that header and, for a public action frame, reads the following action code byte without verifying that either field is present. A truncated frame can therefore make the parser consume up to two bytes past the firmware-declared frame length. If those bytes look like a TDLS discovery response, the malformed frame can spuriously update peer signal state.

Require the category and public action-code fields before reading them. Use sizeof(*ieee_hdr) so the checks and field accesses directly match the firmware four-address layout being parsed before address4 is removed.

---
Changes in v4:
- Match the actual four-address parser layout with sizeof(*ieee_hdr) + 1 and sizeof(*ieee_hdr) + 2, as suggested by Brian.
- Use sizeof(*ieee_hdr) consistently for the corresponding field reads.
- Audit the firmware-length containment concern: both callers reject an rx_pkt_offset plus rx_pkt_length beyond skb->len before entering this helper, so no redundant check is added here.
- Describe the out-of-frame read and possible TDLS signal-state update precisely.

Changes in v3:
- Drop the redundant parser-local header check; the caller already guarantees the complete four-address header after removing the two-byte firmware prefix.

Changes in v2:
- Express the action-field sizes with IEEE80211_MIN_ACTION_SIZE(), accounting for the firmware four-address layout.

---
 drivers/net/wireless/marvell/mwifiex/util.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

index 7d3631d21223..71305efb77ac 100644
@@ -317,10 +317,16 @@ mwifiex_parse_mgmt_packet(struct mwifiex_private *priv, u8 *payload, u16 len,
 
 	switch (stype) {
 	case IEEE80211_STYPE_ACTION:
-		category = *(payload + sizeof(struct ieee80211_hdr));
+		if (len < sizeof(*ieee_hdr) + 1)
+			return -1;
+
+		category = *(payload + sizeof(*ieee_hdr));
 		switch (category) {
 		case WLAN_CATEGORY_PUBLIC:
-			action_code = *(payload + sizeof(struct ieee80211_hdr)
+			if (len < sizeof(*ieee_hdr) + 2)
+				return -1;
+
+			action_code = *(payload + sizeof(*ieee_hdr)
 					+ 1);
 			if (action_code == WLAN_PUB_ACTION_TDLS_DISCOVER_RES) {
 				addr2 = ieee_hdr->addr2;

2.50.1 (Apple Git-155)