[PATCH AUTOSEL 6.18-5.10] media: go7007: avoid undefined shifts in MPEG-4 headers

[Sasha Levin <sashal@kernel.org>]

From: Yousef Alhouseen <alhouseenyousef@gmail.com>

[ Upstream commit 41f82777e547732460290ceb28fcc7c1b95ef192 ]

The byte-alignment paths can call CODE_ADD() with a zero bit
length.  The macro then shifts an int by name.b, which can be 32
after the preceding bit was flushed.

vti_bitlen() can likewise shift a signed int into its sign bit while
searching for the bit length, and the package mask construction can
shift by the full type width.

Make zero-length additions a no-op, derive the bit length with fls(),
and build the mask with GENMASK().

Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

## Phase 1: Commit message forensics

**Step 1.1 — Subject line**
Record: `[media/go7007] [avoid] undefined shifts in MPEG-4 header bit-
packing`. Subsystem is the WIS GO7007 USB MPEG encoder. “Avoid” is a
bug-fix verb: the patch stops C undefined behavior, not adding a
feature.

**Step 1.2 — Tags**
Record:
- `Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>` (author)
- `Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>` (media
  maintainer; applied)
- No `Fixes:`, `Reported-by:`, `Tested-by:`, `Reviewed-by:`, `Acked-
  by:`, `Link:`, or `Cc: stable` (absence of Fixes/Cc-stable is expected
  for this review)

**Step 1.3 — Body**
Record:
- Bug: `CODE_ADD()` can be called with bit length 0; after a flush
  `name.b` can be 32, so `(val) << name.b` is a 32-bit shift of a 32-bit
  int (C UB).
- `vti_bitlen()` uses `(1 << i)` with signed `1` and `i` starting at 31
  (shift into the sign bit).
- Package mask `(1 << vop_time_increment_bitlength) - 1` can shift by
  the full type width.
- Symptoms: UBSAN shift diagnostics; wrong bitstream on arches that mask
  the shift count; possible hang in `vti_bitlen()` when `max_time_incr
  == 0`.
- No version range or crash log in the message.
- Root cause as stated: unguarded shift counts in the MPEG header
  bitpacker.

**Step 1.4 — Hidden bug fix?**
Record: Not disguised. It is an explicit UB/correctness fix. The `fls()`
rewrite is equivalent for every nonzero value and also stops an unsigned
wrap / infinite loop when `max_time_incr == 0`.

---

## Phase 2: Diff analysis

**Step 2.1 — Inventory**
Record:
- File: `drivers/media/usb/go7007/go7007-fw.c` (+15 / −13)
- Functions/macros: `CODE_ADD`, `vti_bitlen()`, `seqhead_to_package()`;
  plus `#include <linux/bitops.h>`
- Scope: single-file surgical fix

**Step 2.2 — Flow change**
Record:
1. `CODE_ADD`: previously always did `name.b -= length; name.a |= (val)
   << name.b`. After: `if (length)` wraps the body, so length-0 is a no-
   op.
2. `vti_bitlen`: previously a countdown from 31 with `(1 << i) - 1`
   masks. After: `fls(max_time_incr)` with `bitlen ?: 1`.
3. `seqhead_to_package`: `(1 << vop_time_increment_bitlength) - 1` →
   `GENMASK(vop_time_increment_bitlength - 1, 0)`.

Affected path is firmware-image construction at MPEG-4 stream start
(`go7007_start_encoder()`), not a generic hot path.

**Step 2.3 — Bug mechanism**
Record: **Type / undefined-shift (UBSAN class)**, with a logic/hang side
effect.

Verified zero-length `CODE_ADD` in `mpeg4_frame_header()` /
`mpeg4_sequence_header()`:

```758:761:drivers/media/usb/go7007/go7007-fw.c
        /* Byte-align with a zero followed by ones */
        i = 8 - (CODE_LENGTH(c) % 8);
        CODE_ADD(c, 0, 1);
        CODE_ADD(c, (1 << (i - 1)) - 1, i - 1);
```

When `CODE_LENGTH % 8 == 7`, `i == 1`, so the second call is
`CODE_ADD(..., 0, 0)`. After the 1-bit add, `name.b` can be 32, then
`(val) << 32` is UB.

Pre-fix `vti_bitlen`:

```c
for (i = 31; (max_time_incr & ((1 << i) - 1)) == max_time_incr; --i)
    ;
return i + 1;
```

`1 << 31` is a signed shift into the sign bit (UB) on every call. If
`max_time_incr == 0`, the condition is always true, `i` is `unsigned
int`, `--i` wraps, and the loop does not terminate.

**Step 2.4 — Fix quality**
Record: Obviously correct and minimal.
- `fls(x)` matches the old loop for all `x > 0` (spot-checked 1, 2,
  30000, 0x80000000).
- `fls(0) ?: 1` returns 1 instead of hanging.
- `GENMASK(n-1, 0)` is the defined way to build an n-bit mask;
  `vti_bitlen()` never returns 0 after the `?: 1` guard, so `GENMASK(-1,
  0)` cannot happen.
- Regression risk is low: no locking, no API, no data-structure change.
  Residual UB not in this patch: `(1 << (i - 1))` in the same padding
  path is safe because `i` is in 1..8.

---

## Phase 3: Git history

**Step 3.1 — Blame**
Record: Buggy `CODE_ADD` / `vti_bitlen` / mask come from
`866b8695d67e83` (“Staging: add the go7007 video driver”, 2008-10-10).
Driver left staging in `7955f03d18d14`. The bug is ~18 years old, not
recent churn.

**Step 3.2 — Fixes: tag**
Record: No `Fixes:` tag. Introducer is the 2008 staging add above. `git
merge-base --is-ancestor 41f82777e5477 HEAD` is true: this snapshot
already contains the fix.

**Step 3.3 — File history**
Record: Recent `go7007-fw.c` commits are unrelated (min/max macros,
SPDX, KASAN frame warning, sparse). Standalone, not `N/M`.

**Step 3.4 — Author**
Record: Yousef Alhouseen is not the media maintainer; nearby work is a
series of small correctness patches. Weight comes from Hans Verkuil’s
SOB (applied to media tree).

**Step 3.5 — Dependencies**
Record: Needs `fls()` and `GENMASK()`, both long-standing
(`<linux/bitops.h>` / `<linux/bits.h>`). No prerequisite series. Can
apply standalone to any tree that still has this file.

---

## Phase 4: Mailing list / external

**Step 4.1 — Original discussion**
Record: `b4 dig -c 41f82777e5477` → https://patch.msgid.link/20260630105
257.54757-1-alhouseenyousef@gmail.com
`b4 dig -a`: **v1 only**, one revision.
Saved mbox: single message, no replies. Hans applied it without public
review comments. No NAK, no reviewer `Cc: stable`.

**Step 4.2 — Recipients (`b4 dig -w`)**
Record: Hans Verkuil, Mauro Chehab, linux-media, linux-kernel. Right
maintainers were on the To/Cc list.

**Step 4.3 — Bug report**
Record: No syzbot/bugzilla/Reported-by. Inspection/UBSAN-style fix, not
a user crash report.

**Step 4.4 — Series**
Record: Standalone v1. Same-day sibling `media: go7007: s2250: propagate
control I/O failures` is a different bug.

**Step 4.5 — Stable list**
Record: lore.kernel.org/stable fetch returned 403. Web search for this
subject on stable found **no** discussion. UNVERIFIED beyond that
search.

---

## Phase 5: Semantic / call graph

**Step 5.1 — Functions**
Record: `CODE_ADD` (macro), `vti_bitlen()`, `seqhead_to_package()`. No
semcode MCP in this environment; used grep.

**Step 5.2 — Callers**
Record:
- `vti_bitlen()` ← `mpeg4_frame_header()`, `mpeg4_sequence_header()`,
  `seqhead_to_package()`
- those ← `gen_mpeg4hdr_to_package()` / `do_special(SPECIAL_SEQHEAD)`
  when `go->format == V4L2_PIX_FMT_MPEG4`
- `go7007_construct_fw_image()` ← `go7007_start_encoder()` ←
  `go7007_start_streaming()` (vb2 `start_streaming`)

**Step 5.3 — Callees**
Record: After the patch, `fls()` and `GENMASK()` only. No alloc/lock in
the changed lines.

**Step 5.4 — Reachability**
Record: Yes, from userspace: open the V4L2 node, `VIDIOC_S_FMT` MPEG-4,
`VIDIOC_STREAMON`. Firmware construction runs with `hw_lock` held.

`VIDIOC_S_PARM` can make `fps_scale` huge:

```485:491:drivers/media/usb/go7007/go7007-v4l2.c
        n = go->sensor_framerate *
                parm->parm.capture.timeperframe.numerator;
        d = 1001 * parm->parm.capture.timeperframe.denominator;
        if (n != 0 && d != 0 && n > d)
                go->fps_scale = (n + d/2) / d;
        else
                go->fps_scale = 1;
```

If `fps_scale > sensor_framerate` (~25025–30030), `max_time_incr == 0`
and the old loop hangs while holding `hw_lock`. Needs device node access
(typically `video` group), not a random unprivileged syscall on machines
without the hardware.

**Step 5.5 — Similar patterns**
Record: MPEG-1 byte-align already skips `CODE_ADD` when `j == 8`. MPEG-4
padding is the length-0 case. No sibling drivers share this macro.

---

## Phase 6: This checkout

**Step 6.1 — Does the buggy code exist here?**
Record: Local tree is **v7.3-rc4** (`7.3.0-rc4`, `f0100363d8c37`). The
driver is present. Current `go7007-fw.c` already has the fix (`#include
<linux/bitops.h>`, `if (length)`, `fls()`, `GENMASK()`). Pre-fix code
lived in this file from 2008 until `41f82777e5477` (2026-07-15). Any
stable kernel that still has unfixed `go7007-fw.c` has the bug.

**Step 6.2 — Backport difficulty**
Record: Clean apply. The bitpacker has been stable since 2008; the hunks
match the historical file. Only extra include is `<linux/bitops.h>`.

**Step 6.3 — Related fixes already here**
Record: This *is* the fix. No earlier alternate fix in the file history.

---

## Phase 7: Subsystem context

**Step 7.1**
Record: `drivers/media/usb/go7007` — USB V4L2 MPEG encoder (WIS GO7007 /
Sensoray 2250, Plextor, etc.). Criticality: **PERIPHERAL** (specific old
capture dongles), but not staging.

**Step 7.2**
Record: Low churn. Recent commits are small leak/error-path fixes.
Mature, rarely touched encoder header generator.

---

## Phase 8: Impact and risk

**Step 8.1 — Who is affected**
Record: Users of `CONFIG_VIDEO_GO7007` with MPEG-4 encode. Hardware IDs
include Sensoray 2250/2251 (`0x1943:0xa250`) and several WIS/Plextor USB
IDs. Not universal.

**Step 8.2 — Trigger**
Record:
- Common: every MPEG-4 `STREAMON` can hit length-0 `CODE_ADD` depending
  on header bit length (UBSAN / UB). Typical `val` is 0, so x86 often
  still produces a no-op.
- Uncommon but real: `VIDIOC_S_PARM` with a large time-per-frame →
  `max_time_incr == 0` → old `vti_bitlen` infinite loop under `hw_lock`.
- Typical `sensor_framerate` is 25025/30000/30030, so `(1 << vti_bitlen)
  - 1` is a ~15-bit shift and not the 32-bit case; the GENMASK change is
  defensive for that site.

**Step 8.3 — Failure mode**
Record:
- Length-0 shift: UBSAN / undefined; functional impact usually none
  because `val == 0`. Severity **MEDIUM** (UBSAN), not a proven oops.
- `vti_bitlen` hang: driver stuck with `hw_lock` held → STREAMON never
  finishes. Severity **HIGH** for that device, not a machine-wide panic.
- Overall: **MEDIUM–HIGH**, not core-kernel CRITICAL.

**Step 8.4 — Risk vs benefit**
Record: Benefit medium for go7007 users (defined behavior, no hang).
Risk **very low** (~15 lines, equivalent `fls()`, maintainer-applied).
Ratio favors backport.

---

## Phase 9: Synthesis

**Step 9.1 — Evidence**

For:
- Real C undefined shifts in reachable MPEG-4 header generation
- Old `vti_bitlen()` can livelock when `fps_scale` is large
- 15/13 lines, one file, obviously correct
- `fls()` is equivalent for all nonzero inputs
- Hans Verkuil signed off
- Bug present since 2008; applies cleanly
- No new API/feature

Against:
- Niche USB encoder, few systems
- No user/syzbot report
- Common-path length-0 add is `val==0`, often a practical no-op
- GENMASK site is not a 32-bit shift with real framerates
- No public review beyond maintainer apply

Unresolved:
- Could not read lore.kernel.org/stable (403)
- No sanitizer log attached to the commit (whether UBSAN actually fired
  is unverified)

**Step 9.2 — Stable rules**
1. Obviously correct and tested? **PASS** — equivalent rewrite;
   maintainer applied v1.
2. Real bug that affects users? **PASS** — UB on MPEG-4 start; hang if
   `max_time_incr==0`.
3. Important (crash/security/corruption/deadlock)? **PASS** — hang under
   `hw_lock` plus UBSAN-class UB. Not a core panic, but a real driver
   DoS/UB, not style cleanup.
4. Small and contained? **PASS** — one file, ~28 lines.
5. No new features/APIs? **PASS**.
6. Applies to this tree’s go7007 code? **PASS** — same file/macros;
   include is standard.

**Step 9.3 — Exception category**
Record: None (not a device ID/quirk/DT/build/docs). It is a normal small
correctness fix.

**Step 9.4 — Decision**
This checkout is mainline 7.3-rc4, which already contains the commit;
the question is whether the fix has stable technical merit for trees
that still have the 2008 bitpacker. It does: small, correct, no
dependencies, fixes defined-behavior bugs and a reachable livelock in
`go7007_start_encoder()`. Risk of regression is negligible.

### What the commit solves for stable users
On MPEG-4 stream start the GO7007 firmware generator packes headers with
`CODE_ADD()`. Padding can request a 0-bit add after a flush (`name.b ==
32`), which is an illegal shift. `vti_bitlen()` always does `1 << 31`
and, if userspace sets a very large frame interval, can spin forever
with `hw_lock` held so `VIDIOC_STREAMON` never returns. The patch skips
zero-length adds, uses `fls()`, and builds the mask with `GENMASK()`.

### Risk vs benefit
Benefit is defined behavior and no encoder-start hang for remaining
GO7007/Sensoray users. Risk is a few lines in a dormant driver with no
API change.

### Concerns
Obscure hardware and no field report. That lowers urgency, not
correctness. No backport rework needed.

---

Verification:
- [Phase 1] Parsed subject/body/tags: only author + Hans Verkuil SOBs;
  no Fixes/Reported-by/Link/Cc-stable
- [Phase 2] `git show --numstat 41f82777e5477`: 15/13, one file; read
  current and `41f82777e5477^` `CODE_ADD` / `vti_bitlen` / mask
- [Phase 2] Confirmed length-0 path: `i = 8 - (CODE_LENGTH % 8)` then
  `CODE_ADD(..., i-1)` when `i==1`
- [Phase 2] Confirmed `fls` equivalence for 1, 2, 30000, 0x80000000;
  `fls(0)?:1` vs old wrap
- [Phase 3] `git blame` / `git log -1 866b8695d67e83`: introduced 2008
  with staging driver
- [Phase 3] `git merge-base --is-ancestor 41f82777e5477 HEAD` → yes
  (already in this 7.3-rc4 snapshot)
- [Phase 3] `git log --oneline --author="Yousef Alhouseen"`: standalone
  vs nearby s2250 patch
- [Phase 4] `b4 dig -c/-a/-w/-m`: v1 only; To Hans/Mauro/linux-media;
  mbox has no replies
- [Phase 4] lore.kernel.org 403; lkml.iu.edu shows the same v1 patch
  text
- [Phase 4] WebSearch stable archive: no hit for this subject
- [Phase 5] grep callers: `vti_bitlen` → MPEG-4 headers → `do_special` →
  `go7007_construct_fw_image` → `go7007_start_encoder` →
  `go7007_start_streaming`
- [Phase 5] Read `vidioc_s_parm`: `fps_scale` unbounded when `n > d`
- [Phase 6] `git describe HEAD` = `v7.3-rc4-27-gf0100363d8c37`; `make
  kernelversion` = 7.3.0-rc4
- [Phase 6] Current file already has the fix; parent file has the old
  loop/mask
- [Phase 7] Kconfig `VIDEO_GO7007`; USB IDs include Sensoray 2250
- [Phase 8] Failure: UB on padding; hang if `max_time_incr==0` under
  `hw_lock`
- UNVERIFIED: whether UBSAN actually reported this in a boot/log;
  stable-list archives beyond web search (403)

**YES**

 drivers/media/usb/go7007/go7007-fw.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/media/usb/go7007/go7007-fw.c b/drivers/media/usb/go7007/go7007-fw.c
index 86ce593e0c546..8cf959c20e30c 100644
--- a/drivers/media/usb/go7007/go7007-fw.c
+++ b/drivers/media/usb/go7007/go7007-fw.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/bitops.h>
 #include <linux/time.h>
 #include <linux/mm.h>
 #include <linux/device.h>
@@ -58,14 +59,16 @@ struct code_gen {
 #define CODE_GEN(name, dest) struct code_gen name = { dest, 0, 32, 0 }
 
 #define CODE_ADD(name, val, length) do { \
-	name.b -= (length); \
-	name.a |= (val) << name.b; \
-	while (name.b <= 24) { \
-		*name.p = name.a >> 24; \
-		++name.p; \
-		name.a <<= 8; \
-		name.b += 8; \
-		name.len += 8; \
+	if (length) { \
+		name.b -= (length); \
+		name.a |= (val) << name.b; \
+		while (name.b <= 24) { \
+			*name.p = name.a >> 24; \
+			++name.p; \
+			name.a <<= 8; \
+			name.b += 8; \
+			name.len += 8; \
+		} \
 	} \
 } while (0)
 
@@ -707,11 +710,10 @@ static int gen_mpeg1hdr_to_package(struct go7007 *go,
 
 static int vti_bitlen(struct go7007 *go)
 {
-	unsigned int i, max_time_incr = go->sensor_framerate / go->fps_scale;
+	unsigned int max_time_incr = go->sensor_framerate / go->fps_scale;
+	int bitlen = fls(max_time_incr);
 
-	for (i = 31; (max_time_incr & ((1 << i) - 1)) == max_time_incr; --i)
-		;
-	return i + 1;
+	return bitlen ?: 1;
 }
 
 static int mpeg4_frame_header(struct go7007 *go, unsigned char *buf,
@@ -1209,7 +1211,7 @@ static int seqhead_to_package(struct go7007 *go, __le16 *code, int space,
 		0xbf08,		fps,
 		0xbf09,		0,
 		0xbff2,		vop_time_increment_bitlength,
-		0xbff3,		(1 << vop_time_increment_bitlength) - 1,
+		0xbff3,		GENMASK(vop_time_increment_bitlength - 1, 0),
 		0xbfe6,		0,
 		0xbfe7,		(fps / 1000) << 8,
 		0,		0,
-- 
2.53.0