Re: [PATCH v3] wifi: mwifiex: validate action frame fixed fields
From: Brian Norris <briannorris@chromium.org>
Date: 2026-08-11 21:40:29
Also in:
lkml, stable
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/ (local)
Link: https://lore.kernel.org/all/20260708195911.84365-8-enderaoelyther@gmail.com/ (local)
Link: https://lore.kernel.org/all/20260723011013.76968-1-enderaoelyther@gmail.com/ (local)
Assisted-by: Codex:gpt-5
Assisted-by: Claude:opus-4.8
Signed-off-by: Zhao Li <redacted>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().
quoted hunk ↗ jump to hunk
--- 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)