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