[PATCH v2] wifi: mwifiex: validate action frame fixed fields

[PATCH v2] wifi: mwifiex: validate action frame fixed fields

[Zhao Li]

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.

Suggested-by: Johannes Berg <johannes@sipsolutions.net>
Fixes: 72e5aa8d2a6d ("mwifiex: support for parsing TDLS discovery frames")
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/all/66f148d83eb9f0970b9abbccc85d1b61244e54ad.camel@sipsolutions.net/
Link: https://lore.kernel.org/all/20260708195911.84365-8-enderaoelyther@gmail.com/
Assisted-by: Codex:gpt-5
Assisted-by: Claude:opus-4.8
Signed-off-by: Zhao Li <enderaoelyther@gmail.com>
---
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(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/util.c b/drivers/net/wireless/marvell/mwifiex/util.c
index 7d3631d212236..4043590137391 100644
--- a/drivers/net/wireless/marvell/mwifiex/util.c
+++ b/drivers/net/wireless/marvell/mwifiex/util.c
@@ -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)

Re: [PATCH v2] wifi: mwifiex: validate action frame fixed fields

[Francesco Dolcini]

On Thu, Jul 23, 2026 at 09:10:13AM +0800, Zhao Li wrote:
> 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.
> 
> Suggested-by: Johannes Berg <johannes@sipsolutions.net>
> Fixes: 72e5aa8d2a6d ("mwifiex: support for parsing TDLS discovery frames")
> Cc: stable@vger.kernel.org
> Link: https://lore.kernel.org/all/66f148d83eb9f0970b9abbccc85d1b61244e54ad.camel@sipsolutions.net/
> Link: https://lore.kernel.org/all/20260708195911.84365-8-enderaoelyther@gmail.com/
> Assisted-by: Codex:gpt-5
> Assisted-by: Claude:opus-4.8
> Signed-off-by: Zhao Li <enderaoelyther@gmail.com>
> ---
> 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(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/util.c b/drivers/net/wireless/marvell/mwifiex/util.c
> index 7d3631d212236..4043590137391 100644
> --- a/drivers/net/wireless/marvell/mwifiex/util.c
> +++ b/drivers/net/wireless/marvell/mwifiex/util.c
> @@ -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;
> +

I am confused, why is this check needed here, and not done in
mwifiex_process_mgmt_packet()? 

Re: [PATCH v2] wifi: mwifiex: validate action frame fixed fields

[Zhao Li]

On Thu, 23 Jul 2026 at 16:41:27 +0200, Francesco Dolcini wrote:
> I am confused, why is this check needed here, and not done in
> mwifiex_process_mgmt_packet()?

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

[PATCH v3] wifi: mwifiex: validate action frame fixed fields

[Zhao Li]

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.

Suggested-by: Johannes Berg <johannes@sipsolutions.net>
Fixes: 72e5aa8d2a6d ("mwifiex: support for parsing TDLS discovery frames")
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/all/66f148d83eb9f0970b9abbccc85d1b61244e54ad.camel@sipsolutions.net/
Link: https://lore.kernel.org/all/20260708195911.84365-8-enderaoelyther@gmail.com/
Link: https://lore.kernel.org/all/20260723011013.76968-1-enderaoelyther@gmail.com/
Assisted-by: Codex:gpt-5
Assisted-by: Claude:opus-4.8
Signed-off-by: Zhao Li <enderaoelyther@gmail.com>
---
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(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/util.c b/drivers/net/wireless/marvell/mwifiex/util.c
index 7d3631d21223..e54a86ecaa33 100644
--- a/drivers/net/wireless/marvell/mwifiex/util.c
+++ b/drivers/net/wireless/marvell/mwifiex/util.c
@@ -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)

Re: [PATCH v3] wifi: mwifiex: validate action frame fixed fields

[Brian Norris]

Hi,

On Fri, Jul 24, 2026 at 04:22:57AM +0800, Zhao Li wrote:
> 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.
> 
> Suggested-by: Johannes Berg <johannes@sipsolutions.net>
> Fixes: 72e5aa8d2a6d ("mwifiex: support for parsing TDLS discovery frames")
> Cc: stable@vger.kernel.org
> Link: https://lore.kernel.org/all/66f148d83eb9f0970b9abbccc85d1b61244e54ad.camel@sipsolutions.net/
> Link: https://lore.kernel.org/all/20260708195911.84365-8-enderaoelyther@gmail.com/
> Link: https://lore.kernel.org/all/20260723011013.76968-1-enderaoelyther@gmail.com/
> Assisted-by: Codex:gpt-5
> Assisted-by: Claude:opus-4.8
> Signed-off-by: Zhao Li <enderaoelyther@gmail.com>

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

> ---
> 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(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/util.c b/drivers/net/wireless/marvell/mwifiex/util.c
> index 7d3631d21223..e54a86ecaa33 100644
> --- a/drivers/net/wireless/marvell/mwifiex/util.c
> +++ b/drivers/net/wireless/marvell/mwifiex/util.c
> @@ -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)

IEEE80211_MIN_ACTION_SIZE() sorta implies we're using 'struct
ieee80211_mgmt' in here. But we're using 'struct ieee80211_hdr' and the
4-address format, in fact.

That disconnect then means you have to awkwardly account for the extra
address by adding ETH_ALEN.

All in all, I kinda prefer the style of v1, where you directly reference
the size of the actual things we're using (sizeof(*ieee_hdr)). It has
the downside of the open-coded "+ 1" and "+ 2", but that's how the
existing parsing works, so IMO it's still better to match that.

(I do see Johannes suggested this in v1, but I'm not sure I agree, now
that I see the result.)

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

If we end up going back to 'sizeof(*ieee_hdr)' approach, I'd suggest
changing this too, for consistency.

Brian

>  					+ 1);
>  			if (action_code == WLAN_PUB_ACTION_TDLS_DISCOVER_RES) {
> -- 
> 2.50.1 (Apple Git-155)
>