[PATCH] wifi: mwifiex: bound uAP association event IEs to the event buffer

[PATCH] wifi: mwifiex: bound uAP association event IEs to the event buffer

[HE WEI (ギカク)]

mwifiex_process_uap_event() handles EVENT_UAP_STA_ASSOC by exposing the
(re)association request IEs that the firmware copies into the event:

	sinfo->assoc_req_ies = &event->data[len];
	len = (u8 *)sinfo->assoc_req_ies - (u8 *)&event->frame_control;
	sinfo->assoc_req_ies_len = le16_to_cpu(event->len) - (u16)len;

event->len is supplied by the device firmware and is never validated,
and the subtraction is unchecked.  assoc_req_ies points into
adapter->event_body[MAX_EVENT_SIZE], a fixed-size array embedded in the
kmalloc()'d struct mwifiex_adapter.

On the ap_11n_enabled path mwifiex_set_sta_ht_cap() walks these IEs with
cfg80211_find_ie(), whose for_each_element() loop dereferences each
element header.  A firmware-reported event->len larger than the bytes
actually received makes assoc_req_ies_len describe IEs that extend past
event_body, so the walk reads out of the adapter slab object -- a
slab-out-of-bounds read (KASAN: slab-out-of-bounds in cfg80211_find_ie).
An event->len smaller than the header instead makes the int subtraction
negative, which wraps to a huge size_t when stored in assoc_req_ies_len.
The same length is handed to cfg80211_new_sta(), so a more modest
over-claim can also copy stale event_body bytes into the
NL80211_CMD_NEW_STATION notification.

A malicious or malfunctioning mwifiex device (USB/SDIO/PCIe) can deliver
such an event while the interface is in AP/uAP mode.

Validate event->len before use: reject a length that underflows the
header or that would place the IEs outside the event_body[] buffer the
event was copied into.  The bound is against event_body[MAX_EVENT_SIZE]
rather than the actually-received length because the transports store the
event differently (USB and SDIO leave the 4-byte event header in
event_skb, PCIe strips it via skb_pull), whereas event_body is the single
fixed buffer all of them copy the event into.  This is the event-path
analogue of the receive-path bounds checks added in commit 119585281617
("wifi: mwifiex: Fix OOB and integer underflow when rx packets").

Fixes: e568634ae7ac ("mwifiex: add AP event handling framework")
Signed-off-by: HE WEI (ギカク) <skyexpoc@gmail.com>
---
 .../net/wireless/marvell/mwifiex/uap_event.c   | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/uap_event.c b/drivers/net/wireless/marvell/mwifiex/uap_event.c
index 679fdae0f001..adca7da29f0f 100644
--- a/drivers/net/wireless/marvell/mwifiex/uap_event.c
+++ b/drivers/net/wireless/marvell/mwifiex/uap_event.c
@@ -126,6 +126,24 @@ int mwifiex_process_uap_event(struct mwifiex_private *priv)
 				sinfo->assoc_req_ies = &event->data[len];
 				len = (u8 *)sinfo->assoc_req_ies -
 				      (u8 *)&event->frame_control;
+
+				/*
+				 * event->len is reported by the device firmware and is not
+				 * otherwise validated.  Reject a length that underflows the
+				 * header, or that would place the association request IEs
+				 * outside the fixed-size event_body[] buffer the event was
+				 * copied into; otherwise the IE walk in
+				 * mwifiex_set_sta_ht_cap() reads past event_body and out
+				 * of the adapter slab object.
+				 */
+				if (le16_to_cpu(event->len) < len ||
+				    (u8 *)&event->frame_control + le16_to_cpu(event->len) >
+				    adapter->event_body + MAX_EVENT_SIZE) {
+					mwifiex_dbg(adapter, ERROR,
+						    "invalid STA assoc event length\n");
+					kfree(sinfo);
+					return -1;
+				}
 				sinfo->assoc_req_ies_len =
 					le16_to_cpu(event->len) - (u16)len;
 			}
-- 
2.54.0

Re: [PATCH] wifi: mwifiex: bound uAP association event IEs to the event buffer

[Francesco Dolcini]

Hello Brian, Johannes

On Mon, Jun 29, 2026 at 09:03:33PM +0900, HE WEI (ギカク) wrote:
> mwifiex_process_uap_event() handles EVENT_UAP_STA_ASSOC by exposing the
> (re)association request IEs that the firmware copies into the event:
> 
> 	sinfo->assoc_req_ies = &event->data[len];
> 	len = (u8 *)sinfo->assoc_req_ies - (u8 *)&event->frame_control;
> 	sinfo->assoc_req_ies_len = le16_to_cpu(event->len) - (u16)len;
> 
> event->len is supplied by the device firmware and is never validated,

I think we received a few patches that are validating the data received
from the firmware (including this one).

I did not review any of them yet, what is your opinion on those?

Should we consider the firmware trust-worth or should we validate
everything we receive from it? Is there some agreement on this topic in
general?

Francesco

Re: [PATCH] wifi: mwifiex: bound uAP association event IEs to the event buffer

[Johannes Berg]

On Fri, 2026-07-10 at 09:21 +0200, Francesco Dolcini wrote:
> I think we received a few patches that are validating the data received
> from the firmware (including this one).
> 
> I did not review any of them yet, what is your opinion on those?
> 
> Should we consider the firmware trust-worth or should we validate
> everything we receive from it? Is there some agreement on this topic in
> general?

I don't really know fully "in general", but I/we generally treat these
as issues. I'm not going to worry too much about them, but I think we
should fix them.

johannes

Re: [PATCH] wifi: mwifiex: bound uAP association event IEs to the event buffer

[skyexpoc]

[-- Attachment #1.1: Type: text/plain, Size: 1710 bytes --]

> Should we consider the firmware trust-worthy or should we validate
> everything we receive from it? Is there some agreement on this topic
> in general?

For mwifiex there's a concrete reason not to trust it: besides the
SDIO/PCIe parts, mwifiex also drives USB dongles (8797/8801/8997), so the
"firmware" can sit on a hot-pluggable, potentially attacker-supplied
device. A malformed event from such a device is the same threat model as
any other malicious USB peripheral, not just a hypothetical firmware bug.

There is also precedent inside mwifiex itself: commit 119585281617
("wifi: mwifiex: Fix OOB and integer underflow when rx packets") already
added exactly these bounds checks to the receive-data path. This patch is
the same class of fix for the uAP event path, which that commit did not
cover.

So I'd say it's consistent with how the driver already treats
firmware-reported lengths and agreed, worth fixing.

Thanks,
HE WEI（ギカク）

> On Friday, Jul 10, 2026 at 4:41 PM, Johannes Berg <johannes@sipsolutions.net (mailto:johannes@sipsolutions.net)> wrote:
> On Fri, 2026-07-10 at 09:21 +0200, Francesco Dolcini wrote:
> > I think we received a few patches that are validating the data received
> > from the firmware (including this one).
> >
> > I did not review any of them yet, what is your opinion on those?
> >
> > Should we consider the firmware trust-worth or should we validate
> > everything we receive from it? Is there some agreement on this topic in
> > general?
>
> I don't really know fully "in general", but I/we generally treat these
> as issues. I'm not going to worry too much about them, but I think we
> should fix them.
>
> johannes

[-- Attachment #1.2: Type: text/html, Size: 2672 bytes --]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 885 bytes --]

Re: [PATCH] wifi: mwifiex: bound uAP association event IEs to the event buffer

[Francesco Dolcini]

On Fri, Jul 10, 2026 at 09:41:26AM +0200, Johannes Berg wrote:
> On Fri, 2026-07-10 at 09:21 +0200, Francesco Dolcini wrote:
> > I think we received a few patches that are validating the data received
> > from the firmware (including this one).
> > 
> > I did not review any of them yet, what is your opinion on those?
> > 
> > Should we consider the firmware trust-worth or should we validate
> > everything we receive from it? Is there some agreement on this topic in
> > general?
> 
> I don't really know fully "in general", but I/we generally treat these
> as issues. I'm not going to worry too much about them, but I think we
> should fix them.

Ack


On Fri, Jul 10, 2026 at 07:44:27PM +0900, skyexpoc wrote:
> > Should we consider the firmware trust-worthy or should we validate
> > everything we receive from it? Is there some agreement on this topic
> > in general?
> 
> For mwifiex there's a concrete reason not to trust it: besides the
> SDIO/PCIe parts, mwifiex also drives USB dongles (8797/8801/8997), so the
> "firmware" can sit on a hot-pluggable, potentially attacker-supplied
> device. A malformed event from such a device is the same threat model as
> any other malicious USB peripheral, not just a hypothetical firmware bug.

...

> So I'd say it's consistent with how the driver already treats
> firmware-reported lengths and agreed, worth fixing.

... and ack.


Johannes, Brian: I will try to review those patches, unless someone
gets to them earlier and they are merged.

Francesco

Re: [PATCH] wifi: mwifiex: bound uAP association event IEs to the event buffer

[Francesco Dolcini]

On Mon, Jun 29, 2026 at 09:03:33PM +0900, HE WEI (ギカク) wrote:
> mwifiex_process_uap_event() handles EVENT_UAP_STA_ASSOC by exposing the
> (re)association request IEs that the firmware copies into the event:
> 
> 	sinfo->assoc_req_ies = &event->data[len];
> 	len = (u8 *)sinfo->assoc_req_ies - (u8 *)&event->frame_control;
> 	sinfo->assoc_req_ies_len = le16_to_cpu(event->len) - (u16)len;
> 
> event->len is supplied by the device firmware and is never validated,

Why not validating event->len instead, when we receive an MWIFIEX_TYPE_EVENT
from the firmware? we could just add a new, validated, u16 event_len, in struct
mwifiex_adapter and after that we can just use it in the code.

and we reject invalid events as soon as possible in the software.


> Fixes: e568634ae7ac ("mwifiex: add AP event handling framework")
> Signed-off-by: HE WEI (ギカク) <skyexpoc@gmail.com>
> ---
>  .../net/wireless/marvell/mwifiex/uap_event.c   | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/uap_event.c b/drivers/net/wireless/marvell/mwifiex/uap_event.c
> index 679fdae0f001..adca7da29f0f 100644
> --- a/drivers/net/wireless/marvell/mwifiex/uap_event.c
> +++ b/drivers/net/wireless/marvell/mwifiex/uap_event.c
> @@ -126,6 +126,24 @@ int mwifiex_process_uap_event(struct mwifiex_private *priv)
>  				sinfo->assoc_req_ies = &event->data[len];
>  				len = (u8 *)sinfo->assoc_req_ies -
>  				      (u8 *)&event->frame_control;
> +
> +				/*
> +				 * event->len is reported by the device firmware and is not
> +				 * otherwise validated.  Reject a length that underflows the
> +				 * header, or that would place the association request IEs
> +				 * outside the fixed-size event_body[] buffer the event was
> +				 * copied into; otherwise the IE walk in
> +				 * mwifiex_set_sta_ht_cap() reads past event_body and out
> +				 * of the adapter slab object.
> +				 */
> +				if (le16_to_cpu(event->len) < len ||
> +				    (u8 *)&event->frame_control + le16_to_cpu(event->len) >
> +				    adapter->event_body + MAX_EVENT_SIZE) {
> +					mwifiex_dbg(adapter, ERROR,
> +						    "invalid STA assoc event length\n");
> +					kfree(sinfo);
> +					return -1;
> +				}

In case we want to keep the change here, I would ask you to make the
code slighly more compact.

just define a

evt_len = le16_to_cpu(event->len)

and use it.

Re: [PATCH] wifi: mwifiex: bound uAP association event IEs to the event buffer

[HE WEI（ギカク）]

> Why not validating event->len instead, when we receive an
> MWIFIEX_TYPE_EVENT from the firmware? we could just add a new,
> validated, u16 event_len, in struct mwifiex_adapter and after that we
> can just use it in the code. and we reject invalid events as soon as
> possible in the software.

Thanks for the review. I don't think that maps onto the code cleanly,
because event->len is not the transport-level event length. It is
struct mwifiex_assoc_event.len (fw.h), a __le16 that lives at offset 8
*inside* the event body and is specific to the EVENT_UAP_STA_ASSOC /
TLV_TYPE_UAP_MGMT_FRAME payload. It measures the 802.11 (re)assoc
frame starting at frame_control, not the whole event.

At the point where we receive an MWIFIEX_TYPE_EVENT (usb.c, sdio.c,
pcie.c -> mwifiex_process_event) the driver only looks at the 4-byte
event_cause and the transport frame length (skb->len, or the PCIe
transfer-header length); it copies the rest of the body into
event_body[] without interpreting it, and each of the ~40 event causes
then casts event_body to its own type-specific struct. So a generic,
receive-time u16 in struct mwifiex_adapter can validate how many bytes
the transport delivered, but it cannot validate event->len there, and
the two are not interchangeable: event->len excludes the
sta_addr/type/len header, so it is about 10 bytes smaller than the
delivered length. Substituting it in the code would compute a wrong
assoc_req_ies_len and re-introduce an over-read.

Storing a validated received length in the adapter is a reasonable
hardening on its own. It would give a tighter bound than
event_body[MAX_EVENT_SIZE] and could help other handlers, but it
touches all three transports (which I can't exercise on real hardware)
and it still needs this per-event check on event->len. I'd rather keep
the security fix minimal and in the one function that knows the
assoc-event layout, and leave the receive-time refactor as a separate
change if you would like it.

> In case we want to keep the change here, I would ask you to make the
> code slightly more compact. just define a evt_len =
> le16_to_cpu(event->len) and use it.

v2 folds event->len into a local evt_len as you suggested, which also
shortens the bounds check. I'll send it as a proper [PATCH v2]; the
hunk is:

if (len != -1) {
u16 evt_len = le16_to_cpu(event->len);

sinfo->assoc_req_ies = &event->data[len];
len = (u8 *)sinfo->assoc_req_ies -
      (u8 *)&event->frame_control;

/*
* event->len is reported by the device firmware
* and is not otherwise validated.  Reject a
* length that underflows the header, or that
* would place the association request IEs
* outside the fixed-size event_body[] buffer the
* event was copied into; otherwise the IE walk
* in mwifiex_set_sta_ht_cap() reads past
* event_body and out of the adapter slab object.
*/
if (evt_len < len ||
    (u8 *)&event->frame_control + evt_len >
    adapter->event_body + MAX_EVENT_SIZE) {
mwifiex_dbg(adapter, ERROR,
    "invalid STA assoc event length\n");
kfree(sinfo);
return -1;
}
sinfo->assoc_req_ies_len = evt_len - (u16)len;
}