Re: [PATCH v2] wifi: mac80211: validate individual TWT params before driver setup

[Johannes Berg]

On Thu, 2026-07-23 at 09:09 +0800, Zhao Li wrote:
> ieee80211_process_rx_twt_action() validates a received S1G TWT setup
> frame only through the control byte and 2-byte req_type before queueing it.

That's not _quite_ true?

static bool
ieee80211_process_rx_twt_action(struct ieee80211_rx_data *rx)
{
...
        switch (mgmt->u.action.action_code) {
        case WLAN_S1G_TWT_SETUP: {
                struct ieee80211_twt_setup *twt;

                if (rx->skb->len < IEEE80211_MIN_ACTION_SIZE(action_code) +
                                   sizeof(struct ieee80211_twt_setup) +
                                   2 /* TWT req_type agrt */)
                        break;

                twt = (void *)mgmt->u.action.s1g.variable;
                if (twt->element_id != WLAN_EID_S1G_TWT)
                        break;

                if (rx->skb->len < IEEE80211_MIN_ACTION_SIZE(action_code) +
                                   3 + /* token + tlv */
                                   twt->length)
                        break;

                return true; /* queue the frame */


But it's all highly confusing.

> An individual agreement can therefore reach ieee80211_s1g_rx_twt_setup()
> with twt->length too short for the full struct ieee80211_twt_params.

And that element isn't even fixed size either...

> The individual path passes twt to drv_add_twt_setup(). Both the tracepoint
> and the driver callback consume the complete parameters block, not merely
> req_type. Do not pass a short individual agreement to the driver.
> Broadcast agreements remain unchanged because they are rejected locally
> after accessing only req_type.
> 
> No in-tree driver both implements .add_twt_setup and exposes S1G
> operation, so this is a robustness fix rather than a fix for an observed
> crash.

I think this also isn't true, because there are some drivers that have
add_twt_setup, and having S1G isn't a necessary condition for this, in
fact this particular path is meant to not be used with S1G but rather
for TWT setup with HE (or later) devices.

Nevertheless, I'm a bit wary of adding this as-is, and I'm not totally
convinced it's a super urgent fix we need right now (-rc5!) even if it's
an out-of-bounds by (at most) 14 bytes.

I'm tempted to feed this https://p.sipsolutions.net/3956a9bfc0ab31b0.txt
into an LLM and tell it add a validation inline for the element size and
move the parsing to the element parser like we do for most other frames,
but I don't really have time to play with it right now. Also, it'd need
more information from the spec (this should be in public versions) about
when each of those fields is present and when it isn't, and the struct
ieee80211_twt_setup I documented there only covers the individual TWT
parameter set field format, not the broadcast one, but I _think_ this is
right...

OK, I'm just going to take this patch into wireless, as ugly as it may
be it seems correct, even if that's far from obvious with the code
patterned this way (not your fault)... we can do further work on top
later. Probably will edit the commit message a bit to not overclaim the
lack of validation though.

johannes