[PATCH AUTOSEL 7.0-5.10] net: hamradio: bpqether: validate frame length in bpq_rcv()

[Sasha Levin]

From: Mashiro Chen <mashiro.chen@xxxxxxxxxxx>

[ Upstream commit 6183bd8723a3eecd2d89cbc506fe938bc6288345 ]

The BPQ length field is decoded as:

  len = skb->data[0] + skb->data[1] * 256 - 5;

If the sender sets bytes [0..1] to values whose combined value is
less than 5, len becomes negative.  Passing a negative int to
skb_trim() silently converts to a huge unsigned value, causing the
function to be a no-op.  The frame is then passed up to AX.25 with
its original (untrimmed) payload, delivering garbage beyond the
declared frame boundary.

Additionally, a negative len corrupts the 64-bit rx_bytes counter
through implicit sign-extension.

Add a bounds check before pulling the length bytes: reject frames
where len is negative or exceeds the remaining skb data.

Acked-by: Joerg Reuter <jreuter@xxxxxxxx>
Signed-off-by: Mashiro Chen <mashiro.chen@xxxxxxxxxxx>
Link: https://patch.msgid.link/20260409024927.24397-2-mashiro.chen@xxxxxxxxxxx
Signed-off-by: Jakub Kicinski <kuba@xxxxxxxxxx>
Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
---

LLM Generated explanations, may be completely bogus:

Now I have all the information I need. Let me compile the complete
analysis.

---

## PHASE 1: COMMIT MESSAGE FORENSICS

**Step 1.1: Subject Line**
- Subsystem: `net: hamradio: bpqether`
- Action verb: "validate" — implies adding a missing safety check (a bug
  fix)
- Summary: Add bounds checking to the frame length parsing in
  `bpq_rcv()`
Record: [net/hamradio/bpqether] [validate] [Add missing frame length
bounds check in receive path]

**Step 1.2: Tags**
- `Acked-by: Joerg Reuter <jreuter@xxxxxxxx>` — Joerg Reuter IS the
  hamradio subsystem maintainer (confirmed from MODULE_AUTHOR)
- `Signed-off-by: Mashiro Chen <mashiro.chen@xxxxxxxxxxx>` — patch
  author
- `Link: https://patch.msgid.link/20260409024927.24397-2-
  mashiro.chen@xxxxxxxxxxx` — lore reference
- `Signed-off-by: Jakub Kicinski <kuba@xxxxxxxxxx>` — netdev maintainer
  applied it
- IMPORTANT: The original submission (from the mbox) includes `Cc:
  stable@xxxxxxxxxxxxxxx` which was stripped during merge
Record: Acked by subsystem maintainer. Originally Cc'd to stable.
Applied by netdev maintainer.

**Step 1.3: Commit Body**
The bug mechanism is clearly described:
- `len = skb->data[0] + skb->data[1] * 256 - 5` can produce a negative
  value if bytes [0..1] sum to < 5
- Passing negative `int` to `skb_trim(unsigned int)` produces a huge
  unsigned value, making it a no-op
- Frame is delivered to AX.25 with untrimmed garbage payload
- Negative `len` also corrupts the 64-bit `rx_bytes` counter via
  implicit sign-extension
Record: Bug is clearly described with specific mechanism. Two distinct
problems: garbage data delivery and stats corruption.

**Step 1.4: Hidden Bug Fix**
This is explicitly a validation/bug fix — "validate" means adding a
missing safety check.
Record: Not hidden — explicitly a bug fix adding missing input
validation.

## PHASE 2: DIFF ANALYSIS

**Step 2.1: Inventory**
- 1 file modified: `drivers/net/hamradio/bpqether.c`
- +3 lines added, 0 removed
- Function modified: `bpq_rcv()`
- Scope: single-file surgical fix
Record: [1 file, +3 lines, bpq_rcv()] [Minimal single-file fix]

**Step 2.2: Code Flow Change**
The single hunk inserts a bounds check after the length calculation:

```190:192:drivers/net/hamradio/bpqether.c
        if (len < 0 || len > skb->len - 2)
                goto drop_unlock;
```

- BEFORE: `len` is calculated and used unconditionally — negative `len`
  passes through
- AFTER: Negative or oversized `len` causes the frame to be dropped
- This is on the data receive path (normal path for incoming frames)
Record: [Before: no validation on computed len → After: reject frames
with invalid len]

**Step 2.3: Bug Mechanism**
Category: **Logic/correctness fix + type conversion bug**
- `len` is `int` (line 152), computed from untrusted network data
- `skb_trim()` takes `unsigned int len` (confirmed from header: `void
  skb_trim(struct sk_buff *skb, unsigned int len)`)
- Negative `int` → huge `unsigned int` → `skb->len > len` is false → no
  trimming occurs
- `dev->stats.rx_bytes += len` with negative `len` corrupts stats via
  sign extension to 64-bit

The fix also checks `len > skb->len - 2` to reject frames claiming more
data than present (the `-2` accounts for the 2 length bytes about to be
pulled).
Record: [Type conversion bug causing no-op trim + stats corruption. Fix
adds proper bounds check.]

**Step 2.4: Fix Quality**
- Obviously correct: a bounds check of `len < 0 || len > skb->len - 2`
  before using `len`
- Minimal/surgical: 3 lines in one location
- No regression risk: rejecting invalid frames cannot harm valid
  operation
- Uses existing `drop_unlock` error path (already well-tested)
Record: [Clearly correct, minimal, no regression risk]

## PHASE 3: GIT HISTORY

**Step 3.1: Blame**
The buggy line (`len = skb->data[0] + skb->data[1] * 256 - 5`) dates to
commit `1da177e4c3f41` — Linus Torvalds' initial Linux import
(2005-04-16). This code has been present in every Linux version ever
released.
Record: [Bug present since initial Linux git commit — affects ALL stable
trees]

**Step 3.2: Fixes Tag**
No explicit `Fixes:` tag. The buggy code predates git history.
Record: [N/A — bug predates git history, all stable trees affected]

**Step 3.3: File History**
Recent changes to `bpqether.c` are all unrelated refactoring (lockdep,
netdev_features, dev_addr_set). None touch the `bpq_rcv()` length
parsing logic. The function `bpq_rcv` hasn't been meaningfully modified
in its length handling since the initial commit.
Record: [No related changes or prerequisites. Standalone fix.]

**Step 3.4: Author**
Mashiro Chen appears to be a contributor fixing input validation issues
(this series fixes two hamradio drivers). The patch was Acked by Joerg
Reuter (subsystem maintainer) and applied by Jakub Kicinski (netdev
maintainer).
Record: [Contributor fix, but Acked by subsystem maintainer and applied
by netdev maintainer — high confidence]

**Step 3.5: Dependencies**
This is patch 1/2 in a series, but both patches are independent
(different files: `bpqether.c` vs `scc.c`). No dependencies.
Record: [Self-contained, no dependencies. Applies standalone.]

## PHASE 4: MAILING LIST RESEARCH

**Step 4.1: Original Discussion**
>From the b4 am output, the thread at
`20260409024927.24397-1-mashiro.chen@xxxxxxxxxxx` contains 5 messages.
This is v2; the change between v1 and v2 for bpqether was only "add
Acked-by: Joerg Reuter" (no code change).

Critical finding from the mbox: **The original patch included `Cc:
stable@xxxxxxxxxxxxxxx`**, indicating the author explicitly nominated it
for stable. This tag was stripped during the merge process (common
netdev practice).
Record: [Original submission Cc'd to stable. v2 adds only Acked-by.
Acked by subsystem maintainer.]

**Step 4.2: Reviewers**
- Acked-by: Joerg Reuter (hamradio subsystem maintainer)
- Applied by: Jakub Kicinski (netdev co-maintainer)
- CC'd to linux-hams mailing list
Record: [Reviewed by the right people]

**Step 4.3-4.5: Bug Report / Stable Discussion**
No external bug report referenced. This appears to be found by code
inspection. The author explicitly Cc'd stable.
Record: [Found by code inspection, author nominated for stable]

## PHASE 5: CODE SEMANTIC ANALYSIS

**Step 5.1: Functions Modified**
Only `bpq_rcv()`.

**Step 5.2: Callers**
`bpq_rcv` is registered as a packet_type handler via
`bpq_packet_type.func = bpq_rcv` (line 93). It is called by the kernel
networking stack for every incoming BPQ ethernet frame (`ETH_P_BPQ`).
This is the main receive path for the driver.
Record: [Called by kernel network stack on every incoming BPQ frame]

**Step 5.3-5.4: Call Chain**
The receive path: network driver → netif_receive_skb → protocol dispatch
→ `bpq_rcv()` → ax25_type_trans → netif_rx.
Any BPQ frame arriving on the network can trigger this. No special
privileges needed to send a malformed Ethernet frame on a local network.
Record: [Reachable from any incoming network frame — attack surface for
local network]

**Step 5.5: Similar Patterns**
The second patch in the series fixes a similar input validation issue in
`scc.c`, suggesting systematic review of hamradio drivers.
Record: [Systematic validation audit by author]

## PHASE 6: CROSS-REFERENCING AND STABLE TREE ANALYSIS

**Step 6.1: Code Exists in Stable?**
Yes. The buggy code (line 188: `len = skb->data[0] + skb->data[1] * 256
- 5`) has been present since the initial commit and exists in ALL stable
trees. The changes since v5.4 and v6.1 to this file are all unrelated
refactoring that don't touch the `bpq_rcv()` length logic.
Record: [Bug exists in ALL stable trees from v5.4 through v7.0]

**Step 6.2: Backport Complications**
None. The surrounding code in `bpq_rcv()` is essentially unchanged. The
fix is a 3-line insertion with no context dependencies on recent
changes.
Record: [Clean apply expected to all stable trees]

**Step 6.3: Related Fixes Already in Stable**
No prior fix for this issue exists.
Record: [No prior fix]

## PHASE 7: SUBSYSTEM CONTEXT

**Step 7.1: Subsystem**
- Path: `drivers/net/hamradio/` — Amateur (ham) radio networking driver
- Criticality: PERIPHERAL (niche driver for ham radio enthusiasts)
- However: it processes network frames and the bug is a missing input
  validation — security relevance
Record: [Peripheral subsystem, but network input validation issue gives
it security relevance]

**Step 7.2: Activity**
The file has had minimal changes. Mature, stable code that rarely gets
touched.
Record: [Very mature code — bug has been present for ~20 years]

## PHASE 8: IMPACT AND RISK ASSESSMENT

**Step 8.1: Who Is Affected**
Users of the BPQ (AX.25-over-Ethernet) hamradio protocol. While niche,
these are real users.
Record: [Driver-specific: ham radio BPQ users]

**Step 8.2: Trigger Conditions**
- Any malformed BPQ frame with length field < 5 triggers the bug
- Can be triggered by any device on the local Ethernet segment (no
  privileges needed)
- Reliably reproducible — no race condition
Record: [Triggered by malformed network frame from local network,
reliably reproducible, no auth needed]

**Step 8.3: Failure Mode**
- Garbage data delivered to AX.25 protocol — potential info leak /
  protocol confusion
- Stats counter corruption (rx_bytes goes wildly negative)
- Severity: MEDIUM-HIGH (garbage data delivery from network, potential
  security implication)
Record: [Garbage data delivery + stats corruption. Severity: MEDIUM-
HIGH]

**Step 8.4: Risk-Benefit**
- BENEFIT: Prevents garbage data delivery and counter corruption from
  malformed frames. Has been broken for 20 years.
- RISK: Extremely low. 3-line bounds check using existing `drop_unlock`
  path. Obviously correct.
Record: [High benefit, very low risk. Excellent ratio.]

## PHASE 9: FINAL SYNTHESIS

**Step 9.1: Evidence Compilation**

FOR backporting:
- Fixes a real input validation bug present since the initial Linux
  commit
- 3-line surgical fix — minimal risk
- Obviously correct bounds check
- Acked by subsystem maintainer (Joerg Reuter)
- Applied by netdev co-maintainer (Jakub Kicinski)
- Author explicitly Cc'd stable@xxxxxxxxxxxxxxx in original submission
- Prevents garbage data delivery to AX.25 from malformed frames
- Prevents stats counter corruption
- Will apply cleanly to all stable trees
- No dependencies, self-contained

AGAINST backporting:
- Niche driver (ham radio) — limited user base
- No crash/panic, "only" garbage data and stats corruption

**Step 9.2: Stable Rules Checklist**
1. Obviously correct and tested? **YES** — trivial bounds check, Acked
   by maintainer
2. Fixes a real bug? **YES** — garbage data delivery + counter
   corruption
3. Important issue? **YES** — input validation on network data
   (security-adjacent)
4. Small and contained? **YES** — 3 lines, 1 file
5. No new features/APIs? **YES** — pure bug fix
6. Applies to stable? **YES** — code unchanged since initial commit

**Step 9.3: Exception Categories**
Not an exception category — this is a standard bug fix.

**Step 9.4: Decision**
This is a textbook stable backport: small, obviously correct input
validation fix for network-facing code, Acked by subsystem maintainer,
explicitly nominated for stable by author, fixes a 20-year-old bug.

## Verification

- [Phase 1] Parsed tags: Acked-by Joerg Reuter, Link to
  patch.msgid.link, SOB from Jakub Kicinski
- [Phase 2] Diff analysis: +3 lines adding bounds check in `bpq_rcv()`
  before `skb_trim`/`skb_pull`
- [Phase 2] Confirmed `skb_trim()` takes `unsigned int len`
  (include/linux/skbuff.h:3321)
- [Phase 3] git blame: buggy code from commit 1da177e4c3f41 (Linus
  initial, 2005-04-16), present in all stable trees
- [Phase 3] git log: no recent changes touch the length handling in
  bpq_rcv()
- [Phase 4] b4 am: found original thread, 5 messages, v2 series with 2
  patches
- [Phase 4] Read mbox: original submission includes `Cc:
  stable@xxxxxxxxxxxxxxx` (line 28 of mbx)
- [Phase 4] Cover letter: v2 changes for bpqether are "no code change;
  add Acked-by"
- [Phase 5] bpq_rcv registered as packet_type handler, called on every
  incoming BPQ frame
- [Phase 6] Code exists in all active stable trees (v5.4+), no
  divergence in affected lines
- [Phase 6] Clean apply expected — surrounding code unchanged
- [Phase 8] Failure mode: garbage data to AX.25 + counter corruption,
  severity MEDIUM-HIGH

**YES**

 drivers/net/hamradio/bpqether.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/hamradio/bpqether.c b/drivers/net/hamradio/bpqether.c
index 045c5177262ea..214fd1f819a1b 100644
--- a/drivers/net/hamradio/bpqether.c
+++ b/drivers/net/hamradio/bpqether.c
@@ -187,6 +187,9 @@ static int bpq_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_ty
 
 	len = skb->data[0] + skb->data[1] * 256 - 5;
 
+	if (len < 0 || len > skb->len - 2)
+		goto drop_unlock;
+
 	skb_pull(skb, 2);	/* Remove the length bytes */
 	skb_trim(skb, len);	/* Set the length of the data */
 
-- 
2.53.0