1:  ec25a426c2b0 ! 1:  e420a88f5049 wifi: nxpwifi: guard the 11ax response handlers against a detached command buffer
    @@ Metadata
      ## Commit message ##
         wifi: nxpwifi: guard the 11ax response handlers against a detached command buffer
     
    -    nxpwifi_wait_queue_complete() detaches the caller-owned data buffer from
    -    the still-current command when the wait is interrupted or times out:
    +    nxpwifi_wait_queue_complete() clears curr_cmd->data_buf when a
    +    synchronous command is interrupted, but nxpwifi_process_cmdresp() still
    +    hands that field to the response handler. The three handlers in 11ax.c
    +    store through it without checking, so a late response oopses.
     
    -            spin_lock_bh(&adapter->nxpwifi_cmd_lock);
    -            if (adapter->curr_cmd == cmd_queued)
    -                    adapter->curr_cmd->data_buf = NULL;
    -            spin_unlock_bh(&adapter->nxpwifi_cmd_lock);
    +    Check the buffer first and return 0 when it is NULL, as the sta_cmd.c
    +    handlers that commit 068986fd6f2a5 ("wifi: nxpwifi: detach sync command
    +    buffer on interrupted wait") covers already do.
     
    -    nxpwifi_cancel_all_pending_cmd() keeps the current command -- "no
    -    recycle probably wait for response" -- so a late response still reaches
    -    nxpwifi_process_cmdresp(), which passes that now-NULL pointer to the
    -    command's cmd_resp callback.
    -
    -    Commit 068986fd6f2a5 ("wifi: nxpwifi: detach sync command buffer on
    -    interrupted wait") introduced that detached state and swept the response
    -    handlers that dereference the buffer unconditionally, adding a "return
    -    0" to nxpwifi_ret_sta_get_chan_info(), nxpwifi_ret_sta_hs_wakeup_reason()
    -    and nxpwifi_ret_sta_robust_coex(). The sweep did not cover 11ax.c.
    -    mwifiex, which the detachment was ported from, has no 11ax or TWT
    -    support, so there was no counterpart there to compare against.
    -
    -    All three handlers in 11ax.c dereference the buffer with no check:
    -
    -      nxpwifi_ret_11ax_cfg() takes &ax_cfg->he_cap_cfg and stores through it
    -      at "he_cap->id = tlv_type". nxpwifi_set_11ax_status() declares the
    -      struct nxpwifi_11ax_he_cfg on the stack and issues HOST_CMD_11AX_CFG
    -      synchronously while bringing an AP up, so this one is reachable.
    -
    -      nxpwifi_ret_twt_cfg() stores at "twt_cfg->sub_id = ...". The twt_setup,
    -      twt_teardown, twt_report and twt_information debugfs files each pass a
    -      stack struct nxpwifi_twt_cfg to a synchronous HOST_CMD_TWT_CFG, so this
    -      one is reachable too.
    -
    -      nxpwifi_ret_11ax_cmd() stores at "ax_cmd->sub_id = ...". Nothing in the
    -      tree sends HOST_CMD_11AX_CMD, so this one is guarded for symmetry only.
    -
    -    Return 0 and drop the response, the way the three handlers the original
    -    sweep covered already do.
    +    This is a local denial of service: it needs an interrupted synchronous
    +    command and a late response, both on the local host.
     
         Fixes: 73b01e57ed3e ("wifi: nxp: add nxpwifi driver for IW61x")
         Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
2:  a3dd16afd7a6 ! 2:  5a07b1ec968d wifi: nxpwifi: read the 11AX_CFG extension ID at the right offset
    @@ Metadata
      ## Commit message ##
         wifi: nxpwifi: read the 11AX_CFG extension ID at the right offset
     
    -    nxpwifi_ret_11ax_cfg() reads the extension ID of a TLV one byte past
    -    where it is:
    -
    -            ext_id = *((u8 *)header + sizeof(*header) + 1);
    -
    -    Everything else in the driver puts the extension ID at the first payload
    -    byte, sizeof(*header) in. The send side of this very command does:
    -
    -            memcpy(he_cfg->tlv + sizeof(*header),
    -                   &ax_cfg->he_cap_cfg.ext_id,
    -                   ax_cfg->he_cap_cfg.len);
    -
    -    struct nxpwifi_11ax_he_cap_cfg has ext_id at offset 4, after the u16 id
    -    and the u16 len, and so do struct nxpwifi_ie_types_he_cap and struct
    -    hw_spec_extension. nxpwifi_ret_sta_get_hw_spec() parses the same
    -    TLV_TYPE_EXTENSION_ID out of a firmware response by casting to struct
    -    hw_spec_extension and reading ->ext_id, i.e. at sizeof(*header). The
    -    caller in nxpwifi_set_11ax_status() also writes the extension ID there,
    -    at ax_cfg.data + 4. These two lines are the only place in the driver
    -    that adds one.
    -
    -    The consequences are that the HE-capability test is made on the byte
    -    after the extension ID, which is mac_cap_info[0], so a capability TLV is
    -    rejected and some other TLV may be accepted in its place; that the copy
    -    into he_cap->ext_id lands one byte off, leaving ext_id holding
    -    mac_cap_info[0] and every later field shifted; and that the copy reads
    -    one byte past the end of the TLV payload the response declared, since it
    -    takes tlv_len bytes starting at payload[1]. The other copy in the same
    -    function, into priv->user_he_cap, uses the unshifted offsets and so
    -    disagrees with the first one. priv->user_he_cap is what
    -    nxpwifi_update_11ax_cap() and nxpwifi_fill_he_cap_tlv() read back as a
    -    struct nxpwifi_ie_types_he_cap, with ext_id at offset 4.
    -
    -    Drop the two plus-ones. With the correct offset the loop condition
    -    "left_len > sizeof(*header)" already keeps the extension ID read inside
    -    the declared response; it is the plus-one that needed one byte more than
    -    the loop guarantees.
    -
    -    Also require a payload before reading the extension ID out of it. A
    -    zero-length extension TLV carries no extension ID, and without the test
    -    the first byte of whatever follows it is read as one.
    +    The send side writes the extension ID as the first payload byte of the
    +    TLV in nxpwifi_cmd_11ax_cfg(), but nxpwifi_ret_11ax_cfg() reads it one
    +    byte later and copies the HE capability from the same wrong offset. The
    +    capability test runs on the wrong byte and the copy reads one byte past
    +    the declared payload.
    +
    +    Read the extension ID and copy the capability from the first payload
    +    byte. A zero-length extension TLV has no payload to read an extension ID
    +    out of, so skip it.
     
         Fixes: 73b01e57ed3e ("wifi: nxp: add nxpwifi driver for IW61x")
         Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
3:  853c42a7069d ! 3:  b6d6622a4554 wifi: nxpwifi: bound the TLV lengths in the 11AX_CFG response
    @@ Metadata
      ## Commit message ##
         wifi: nxpwifi: bound the TLV lengths in the 11AX_CFG response
     
    -    nxpwifi_ret_11ax_cfg() walks the TLVs the firmware returns for
    -    HOST_CMD_11AX_CFG and copies one of them into two fixed-size
    -    destinations. Nothing bounds any of the three lengths involved.
    +    nxpwifi_ret_11ax_cfg() walks the TLVs the firmware returns and copies an
    +    HE capability into fixed-size buffers without bounding any length. A
    +    response shorter than the fixed part wraps the u16 walk length, and
    +    header->len is used as a memcpy length without being checked against the
    +    destination.
     
    -    The walk length comes straight out of the response:
    +    Reject a response too short for the fixed part, reject a TLV longer than
    +    the bytes left, and reject an HE capability that does not fit its
    +    destination. Select the destination buffer first, by band, then validate
    +    and copy that one.
     
    -            left_len = le16_to_cpu(resp->size) - sizeof(*he_cfg) - S_DS_GEN;
    -
    -    left_len is a u16 and the two sizes add up to 11, so a response claiming
    -    fewer than 11 bytes makes the subtraction wrap instead of ending the
    -    walk, and the loop condition is only "left_len > sizeof(*header)".
    -
    -    Inside the loop, header->len is used as a memcpy length twice without
    -    being compared to what is left or to what the destinations hold:
    -
    -            memcpy((u8 *)&he_cap->ext_id,
    -                   (u8 *)header + sizeof(*header),
    -                   tlv_len);
    -            memcpy(priv->user_he_cap, (u8 *)header, sizeof(*header) + tlv_len);
    -
    -    The first destination runs from ext_id to the end of struct
    -    nxpwifi_11ax_he_cap_cfg, which is 50 bytes; the second is
    -    priv->user_he_cap[HE_CAP_MAX_SIZE], which is 54. header->len is 16 bits.
    -    nxpwifi_set_11ax_status() declares the struct nxpwifi_11ax_he_cfg this
    -    writes into on the stack and issues the command synchronously, so the
    -    frame is still live when the response is parsed.
    -
    -    Do what the driver already does elsewhere. nxpwifi_ret_sta_get_hw_spec()
    -    checks that the response is longer than its fixed part before
    -    subtracting, and nxpwifi_uap_parse_tail_ies() rejects an element longer
    -    than the bytes remaining. Add the matching three checks here: refuse a
    -    response that cannot hold the fixed part, refuse a TLV that claims more
    -    than the remaining length, and refuse an HE-capability TLV that does not
    -    fit either destination. A TLV whose length is within those bounds is
    -    copied exactly as before, including one that fills both destinations
    -    completely.
    -
    -    This is not a claim of a reachable out-of-bounds write. The lengths come
    -    from the device firmware, and whether it can emit these values is not
    -    something the tree answers. The arithmetic reaches past both
    -    destinations, the idiom is already in the driver, and the checks are
    -    cheap.
    -
    -    The driver has not appeared in a released kernel, so there is nothing to
    -    backport to and no stable tag.
    +    The lengths come from device firmware, so this is not a demonstrated
    +    out-of-bounds write. The driver has not appeared in a released kernel,
    +    so there is no stable tag.
     
         Fixes: 73b01e57ed3e ("wifi: nxp: add nxpwifi driver for IW61x")
         Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
    @@ drivers/net/wireless/nxp/nxpwifi/11ax.c: int nxpwifi_ret_11ax_cfg(struct nxpwifi
      		if (tlv_type == TLV_TYPE_EXTENSION_ID && tlv_len >= 1) {
      			ext_id = *((u8 *)header + sizeof(*header));
      			if (ext_id == WLAN_EID_EXT_HE_CAPABILITY) {
    ++				u8 *user_cap, *user_cap_len;
    ++				size_t user_cap_size;
    ++
    ++				if (he_cfg->band_config & BIT(1)) {
    ++					user_cap = priv->user_he_cap;
    ++					user_cap_len = &priv->user_he_cap_len;
    ++					user_cap_size =
    ++						sizeof(priv->user_he_cap);
    ++				} else {
    ++					user_cap = priv->user_2g_he_cap;
    ++					user_cap_len =
    ++						&priv->user_2g_he_cap_len;
    ++					user_cap_size =
    ++						sizeof(priv->user_2g_he_cap);
    ++				}
    ++
     +				if (tlv_len > sizeof(*he_cap) -
     +				    offsetof(struct nxpwifi_11ax_he_cap_cfg,
     +					     ext_id) ||
    -+				    sizeof(*header) + tlv_len >
    -+				    sizeof(priv->user_he_cap) ||
    -+				    sizeof(*header) + tlv_len >
    -+				    sizeof(priv->user_2g_he_cap))
    ++				    sizeof(*header) + tlv_len > user_cap_size)
     +					return -EINVAL;
     +
      				he_cap->id = tlv_type;
      				he_cap->len = tlv_len;
      				memcpy((u8 *)&he_cap->ext_id,
    + 				       (u8 *)header + sizeof(*header),
    + 				       tlv_len);
    +-				if (he_cfg->band_config & BIT(1)) {
    +-					memcpy(priv->user_he_cap,
    +-					       (u8 *)header,
    +-					       sizeof(*header) + tlv_len);
    +-					priv->user_he_cap_len =
    +-						sizeof(*header) + tlv_len;
    +-				} else {
    +-					memcpy(priv->user_2g_he_cap,
    +-					       (u8 *)header,
    +-					       sizeof(*header) + tlv_len);
    +-					priv->user_2g_he_cap_len =
    +-						sizeof(*header) + tlv_len;
    +-				}
    ++				memcpy(user_cap, (u8 *)header,
    ++				       sizeof(*header) + tlv_len);
    ++				*user_cap_len = sizeof(*header) + tlv_len;
    + 			}
    + 		}
    +