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  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  3: 853c42a7069d < -: ------------ wifi: nxpwifi: bound the TLV lengths in the 11AX_CFG response -: ------------ > 3: b6d6622a4554 wifi: nxpwifi: bound the TLV lengths in the 11AX_CFG response