Linux wireless drivers development
 help / color / mirror / Atom feed
From: Sasha Levin <sashal@kernel.org>
To: patches@lists.linux.dev, stable@vger.kernel.org
Cc: Amir Mohammad Jahangirzad <a.jahangirzad@gmail.com>,
	Johannes Berg <johannes.berg@intel.com>,
	Sasha Levin <sashal@kernel.org>,
	kvalo@kernel.org, libertas-dev@lists.infradead.org,
	linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [PATCH AUTOSEL 7.0-5.10] wifi: libertas: fix integer underflow in process_cmdrequest()
Date: Mon, 11 May 2026 18:19:11 -0400	[thread overview]
Message-ID: <20260511221931.2370053-12-sashal@kernel.org> (raw)
In-Reply-To: <20260511221931.2370053-1-sashal@kernel.org>

From: Amir Mohammad Jahangirzad <a.jahangirzad@gmail.com>

[ Upstream commit 3994b4afd521d60e47e012fe2ed7b606aaec370b ]

The existing validation only checks if recvlength exceeds
LBS_CMD_BUFFER_SIZE, but doesn't check the lower bound. When a
USB device sends a response shorter than MESSAGE_HEADER_LEN, the
subtraction (recvlength - MESSAGE_HEADER_LEN) wraps to a huge
value, causing memcpy to corrupt the heap.
Add the same lower bound check that libertas_tf already has.

Signed-off-by: Amir Mohammad Jahangirzad <a.jahangirzad@gmail.com>
Link: https://patch.msgid.link/20260418004247.368944-1-a.jahangirzad@gmail.com
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

## Phase Walkthrough
### Phase 1: Commit Message Forensics
Step 1.1 Record: Subsystem is `wifi: libertas`; action verb is `fix`;
claimed intent is to prevent integer underflow in
`process_cmdrequest()`.

Step 1.2 Record: Tags present are `Signed-off-by: Amir Mohammad
Jahangirzad`, `Link: https://patch.msgid.link/20260418004247.368944-1-
a.jahangirzad@gmail.com`, and `Signed-off-by: Johannes Berg`. No
`Fixes:`, `Reported-by:`, `Tested-by:`, `Reviewed-by:`, `Acked-by`, or
`Cc: stable` tag was present in the supplied commit message or original
posted patch.

Step 1.3 Record: The commit body describes a concrete memory corruption
bug: `recvlength` is only checked against the upper bound, then
`recvlength - MESSAGE_HEADER_LEN` is stored in `priv->resp_len[i]` and
used as the `memcpy()` length. If a USB device supplies fewer than 4
bytes, the subtraction becomes negative and is converted to a huge
unsigned copy length. Symptom/failure mode: heap/driver memory
corruption from `memcpy()`. Version information: none in the message.
Root cause: missing lower-bound validation.

Step 1.4 Record: This is not hidden; it is explicitly a memory-safety
fix. It matches the same already-present guard in `libertas_tf`.

### Phase 2: Diff Analysis
Step 2.1 Record: One file changed:
`drivers/net/wireless/marvell/libertas/if_usb.c`, 3 insertions and 2
deletions. Modified function: `process_cmdrequest()`. Scope: single-file
surgical fix.

Step 2.2 Record: Before, `process_cmdrequest()` rejected only
`recvlength > LBS_CMD_BUFFER_SIZE`; lengths `1..3` passed and produced
`recvlength - MESSAGE_HEADER_LEN`. After, it rejects `recvlength <
MESSAGE_HEADER_LEN` as well as overlarge responses. This affects the USB
command-response receive path.

Step 2.3 Record: Bug category is memory safety, specifically integer
underflow leading to oversized `memcpy()`. Verified details:
`MESSAGE_HEADER_LEN` is 4, `resp_len` is `u32`, `resp_buf` is `u8
resp_buf[2][LBS_UPLD_SIZE]`, and `LBS_UPLD_SIZE` is 2312. A negative
subtraction assigned to `u32` becomes a huge length, far beyond the
destination buffer.

Step 2.4 Record: Fix quality is high: minimal bounds check, no API
change, no new behavior except rejecting malformed command responses.
Regression risk is very low; valid command responses must already
include the 4-byte command type/header.

### Phase 3: Git History Investigation
Step 3.1 Record: `git blame` shows the upper-bound check came from
`ddac452680a516` in the v2.6.25-rc1 era, and the `resp_len = recvlength
- MESSAGE_HEADER_LEN` plus `memcpy()` flow came from `7919b89c8276` in
the v2.6.26-rc1 era. This code is old and widely present.

Step 3.2 Record: No `Fixes:` tag is present in the candidate, so there
is no specific tagged introducing commit to follow. Blame nevertheless
identifies the relevant old code.

Step 3.3 Record: Recent file history includes unrelated cleanup/fix
commits such as `3968e81ba644` changing skb free placement and
`d66676e6ca96` fixing a warning in `usb_tx_block()`. I found no
prerequisite commit needed for this bounds check.

Step 3.4 Record: `git log --author='Amir Mohammad Jahangirzad'` found no
prior local commits in this Marvell wireless subtree. The final signoff
is from Johannes Berg; `MAINTAINERS` lists Johannes Berg as wireless
maintainer, while the Libertas driver itself is marked orphaned under
`linux-wireless` and `libertas-dev`.

Step 3.5 Record: Dependencies found: none. The patch uses existing local
constants and mirrors the already-existing `libertas_tf` check.

### Phase 4: Mailing List And External Research
Step 4.1 Record: No commit hash was available in local history, so `b4
dig -c` could not be used successfully; `b4 dig -c
20260418004247.368944-1-a.jahangirzad@gmail.com` failed because it
expects a commit. Fallback `b4 mbox` and the lore mirror found the
original patch at `https://yhbt.net/lore/lkml/20260418004247.368944-1-
a.jahangirzad@gmail.com/T/`. The thread has one message and no replies.
`b4 mbox -c` found no newer revision in the thread.

Step 4.2 Record: Original recipients included Johannes Berg, Kees Cook,
Ingo Molnar, Johan Hovold, `linux-wireless`, `libertas-dev`, and `linux-
kernel`. No reviewer replies, NAKs, or explicit stable nominations were
present in the fetched thread.

Step 4.3 Record: No `Reported-by` or bug-report link was present. I
found no separate public bug report for this exact issue. The message
itself provides the failure mechanism.

Step 4.4 Record: Related precedent exists: commit `3348ef6a6a126` fixed
the identical underflow in `libertas_tf: process_cmdrequest()`, with
message “If recvlength is less than MESSAGE_HEADER_LEN (4) we would end
up corrupting memory.” That analogous fix was later carried in stable
review postings for 4.19 and 3.16.

Step 4.5 Record: Web searches found the exact candidate posting and
stable history for the analogous `libertas_tf` fix, but no exact stable
discussion for this new `libertas` patch.

### Phase 5: Code Semantic Analysis
Step 5.1 Record: Modified function: `process_cmdrequest()`.

Step 5.2 Record: Caller is `if_usb_receive()`, reached as the receive
URB completion callback installed by `usb_fill_bulk_urb()` through
`if_usb_submit_rx_urb()`.

Step 5.3 Record: Key callees are `memcpy()`, `dev_kfree_skb_irq()`, and
`lbs_notify_command_response()`. The command response is later consumed
by the main thread through `lbs_process_command_response()`.

Step 5.4 Record: Reachability is verified through USB receive
completion: a Libertas USB device response with type `CMD_TYPE_REQUEST`
reaches `process_cmdrequest()`. The triggering input is device-
controlled USB receive data, so this is reachable with affected hardware
or a malicious/faulty USB device.

Step 5.5 Record: Similar pattern found in `libertas_tf`; that sibling
driver already has the exact lower-bound check. `if_sdio` and `if_spi`
use different response formats and do not subtract `MESSAGE_HEADER_LEN`
in the same way.

### Phase 6: Cross-Referencing And Stable Tree Analysis
Step 6.1 Record: Representative tags `v3.16`, `v4.14`, `v4.19`, `v5.4`,
`v5.10`, `v5.15`, `v6.1`, `v6.6`, `v6.12`, and newer `v6.13` through
`v6.17` all contain the buggy `process_cmdrequest()` pattern without the
lower-bound check. The bug dates back to at least the v2.6.26-rc1 era.

Step 6.2 Record: Expected backport difficulty is low for modern stable
trees: the same context is present in representative stable tags, and
`git apply --check` succeeds against the current tree. Very old trees
before the Marvell directory move may need path/context adjustment, as
verified by older tags using `drivers/net/wireless/libertas/if_usb.c`.

Step 6.3 Record: No exact related fix for `libertas` was found in local
history. The sibling `libertas_tf` fix exists and was stable-backported.

### Phase 7: Subsystem And Maintainer Context
Step 7.1 Record: Subsystem is wireless driver code under
`drivers/net/wireless/marvell/libertas`. Criticality: driver-specific,
but memory corruption in a kernel USB receive path is high severity for
affected systems.

Step 7.2 Record: Subsystem activity is low-to-moderate; recent history
shows occasional fixes and cleanups. `MAINTAINERS` marks Marvell
Libertas as orphaned, with `linux-wireless` and `libertas-dev` lists.

### Phase 8: Impact And Risk Assessment
Step 8.1 Record: Affected population is config- and hardware-specific:
systems with `CONFIG_LIBERTAS_USB` and Marvell Libertas 8388 USB
devices, or systems exposed to a malicious USB device matching the
driver IDs.

Step 8.2 Record: Trigger condition is a USB command response shorter
than `MESSAGE_HEADER_LEN` but with `CMD_TYPE_REQUEST` dispatch. This is
device-controlled. I did not verify an unprivileged software-only
trigger; physical or device-level control is the verified trigger model.

Step 8.3 Record: Failure mode is heap/driver memory corruption from
oversized `memcpy()`. Severity: HIGH to CRITICAL, because kernel memory
corruption can crash the system and can have security implications.

Step 8.4 Record: Benefit is high for affected systems: prevents kernel
memory corruption from malformed USB responses. Risk is very low: 3-line
effective validation change in one driver function, with precedent in
sibling driver.

### Phase 9: Final Synthesis
Step 9.1 Record: Evidence for backporting: real memory corruption bug;
old code present across many stable release tags; tiny surgical fix; no
new API or feature; exact validation already used by `libertas_tf`;
analogous `libertas_tf` fix was stable-backported. Evidence against:
driver-specific and no external bug report or tested-by tag. Unresolved:
exact final upstream commit hash was not present in local history, so
`b4 dig -c <commit_hash>` could not be completed.

Step 9.2 Record:
1. Obviously correct and tested? Obviously correct by code inspection;
   no explicit `Tested-by`.
2. Fixes a real bug? Yes, verified underflow into oversized `memcpy()`.
3. Important issue? Yes, kernel memory corruption.
4. Small and contained? Yes, one function in one file.
5. No new features/APIs? Yes.
6. Can apply to stable trees? Yes for modern representative tags with
   likely clean/minor context; old pre-directory-move trees may need
   path adjustment.

Step 9.3 Record: No exception category such as device ID, quirk, DT,
build, or docs applies. This is a direct memory-safety bug fix.

Step 9.4 Record: Stable backport decision is YES. The fix prevents
kernel memory corruption in an old, stable-present USB driver path with
extremely low regression risk.

## Verification
- Phase 1: Parsed supplied tags and fetched original patch thread with
  `b4 mbox`; confirmed no `Fixes`, `Reported-by`, `Tested-by`,
  `Reviewed-by`, `Acked-by`, or stable tag.
- Phase 2: Read `drivers/net/wireless/marvell/libertas/if_usb.c`,
  `dev.h`, and `defs.h`; confirmed `MESSAGE_HEADER_LEN`, `resp_len`,
  `resp_buf`, and `memcpy()` behavior.
- Phase 3: Ran `git blame` on `process_cmdrequest()`; identified old
  introducing history around `ddac452680a516` and `7919b89c8276`.
- Phase 3: Ran `git describe --contains`; confirmed relevant code dates
  to v2.6.25/v2.6.26-era history.
- Phase 4: `b4 dig` with the message-id failed because no local commit
  hash was available; recorded as unresolved.
- Phase 4: `b4 mbox` fetched the patch thread; `b4 mbox -c` found one
  message and no newer revision.
- Phase 4: Fetched lore mirror and stable archive pages; confirmed
  analogous `libertas_tf` fix and stable review postings.
- Phase 5: Traced callers from `if_usb_receive()` through
  `usb_fill_bulk_urb()` and `if_usb_submit_rx_urb()`.
- Phase 6: Checked release tags from `v3.16` through representative
  active v6 tags; confirmed the buggy code exists broadly.
- Phase 6: Ran `git apply --check` against the current tree; patch
  applies cleanly there.
- Phase 7: Checked `MAINTAINERS`; confirmed Libertas is orphaned under
  wireless lists and Johannes Berg is listed for wireless maintenance.
- Phase 8: Verified trigger is USB device-controlled receive data; no
  unprivileged software-only trigger was verified.

**YES**

 drivers/net/wireless/marvell/libertas/if_usb.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas/if_usb.c b/drivers/net/wireless/marvell/libertas/if_usb.c
index 8a6bf1365cfab..42d3fd32ece28 100644
--- a/drivers/net/wireless/marvell/libertas/if_usb.c
+++ b/drivers/net/wireless/marvell/libertas/if_usb.c
@@ -625,9 +625,10 @@ static inline void process_cmdrequest(int recvlength, uint8_t *recvbuff,
 	unsigned long flags;
 	u8 i;
 
-	if (recvlength > LBS_CMD_BUFFER_SIZE) {
+	if (recvlength < MESSAGE_HEADER_LEN ||
+	    recvlength > LBS_CMD_BUFFER_SIZE) {
 		lbs_deb_usbd(&cardp->udev->dev,
-			     "The receive buffer is too large\n");
+			     "The receive buffer is invalid: %d\n", recvlength);
 		kfree_skb(skb);
 		return;
 	}
-- 
2.53.0


  parent reply	other threads:[~2026-05-11 22:19 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20260511221931.2370053-1-sashal@kernel.org>
2026-05-11 22:19 ` [PATCH AUTOSEL 7.0-5.15] wifi: nl80211: re-check wiphy netns in nl80211_prepare_wdev_dump() continuation Sasha Levin
2026-05-11 22:19 ` Sasha Levin [this message]
2026-05-11 22:19 ` [PATCH AUTOSEL 7.0-5.10] wifi: nl80211: require CAP_NET_ADMIN over the target netns in SET_WIPHY_NETNS Sasha Levin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260511221931.2370053-12-sashal@kernel.org \
    --to=sashal@kernel.org \
    --cc=a.jahangirzad@gmail.com \
    --cc=johannes.berg@intel.com \
    --cc=kvalo@kernel.org \
    --cc=libertas-dev@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=patches@lists.linux.dev \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox