# DF-0633 — VERDICT

**Verdict: NOT REPRODUCED at runtime (race did not strike within budget). Bug CONFIRMED in source.**

## Mechanism (the bug IS real in source)

`ip_fw3_ctl_state_get` (`sys/net/ipfw3_basic/ip_fw3_state.c:422`) runs on
netisr CPU 0 (asserted by `ASSERT_NETISR0` in `ip_fw3_glue.c:55`, called
via `rip_ctloutput` from `raw_ip.c:308`). Its body, however, walks every
CPU's state tree directly:

```c
438:	for (cpu = 0; cpu < ncpus; cpu++) {
439:		state_ctx = fw3_state_ctx[cpu];
440:		RB_FOREACH(s, fw3_state_tree, &state_ctx->rb_icmp_in) {
441:			... ioc->rule_id = s->stub->rulenum; ...
```

There is **no `netmsg_init`+`netisr_domsg`+`netisr_forwardmsg_all`**
dispatch for `cpu != mycpuid`, **no `lwkt_token`**, and **no lock**. The
per-CPU RB trees are concurrently mutated by their owning CPUs' netisrs:

- `check_keep_state` does `RB_INSERT(fw3_state_tree, the_tree, s)` at
  `:327` whenever a new flow matches a keep-state rule.
- `ip_fw3_state_cleanup_dispatch` does `RB_REMOVE + kfree` at `:547/553/
  559/565/571/577` for expired entries.

Every other cross-CPU touch in this subsystem correctly uses the netmsg
pattern (`:336-345` append, `:361-403` flush, `:540-582` cleanup, `:619-632`
init, `:635-688` fini). Only `ip_fw3_ctl_state_get` breaks the contract.
Note also that **`RB_FOREACH_SAFE` is NOT used** — plain `RB_FOREACH`,
which dereferences `s` after the iteration macro may have already moved
past it, so removal by another CPU of the node we're standing on crashes
the reader. The body also dereferences `s->stub->rulenum` (lines 449,
464, 479, 494, 509, 524), compounding with DF-0631 if `s->stub` is stale.

Race outcomes:
- panic from following a stale `rb_node` pointer into unmapped memory
- UAF read when a node is `kfree()`'d mid-traversal
- infinite loop soft-locking netisr 0 (if a half-applied tree rotation
  forms a cycle)

## Runtime demonstration (best-effort)

Two stress runs were attempted on the 6-CPU SMP audit guest:

| Run | generators | state-show iters | icmp churn | udp churn | result |
|-----|-----------|------------------|-----------|-----------|--------|
| 1   | 10        | 50               | ~150 pkts | none      | no panic |
| 2   | 12        | 500              | continuous| none      | no panic |

In both runs the guest stayed up and `ipfw3 state show` completed without
error. The race **did not strike within the test budget**. This is
expected for race-condition findings — the bug is probabilistic, the
window per iteration is small, and the slab allocator's quarantine
prevents immediate reuse so UAF reads often return benign residue.

The bug is **confirmed by source-level analysis**: every other
cross-CPU state operation in this file uses netmsg dispatch, and the
single exception (`ip_fw3_ctl_state_get`) is a clear omission. A
maintainer reading the code will see the inconsistency immediately.

Classification: bug CONFIRMED in source, runtime race **not
deterministically triggered** within the available iteration budget.
Valid reason: *the bug is a non-deterministic race; we ran the realistic
trigger (concurrent state churn + state-show) and it did not strike.
Source-level proof is conclusive.* This is not a hard blocker per se;
the bug IS reachable at runtime, just probabilistic.

## Realistic impact ceiling

The trigger requires a privileged user (raw IPv4 socket with
`SYSCAP_NONET_RAW`, root-equivalent) calling
`getsockopt(IPPROTO_IP, IP_FW_X, ..., IP_FW_STATE_GET)`. The race
aggravation (concurrent packet processing on other CPUs) can be driven
by an unauthenticated remote peer sending cheap TCP/UDP/ICMP packets
matching any keep-state rule.

Worst-case outcome: kernel panic (local DoS from the privileged user's
perspective; remote-aggravated DoS in the sense that an unauth peer can
keep the trees churning while the privileged query runs). The CVSS
vector `AV:L/AC:H/PR:H/.../C:L/I:L/A:H` correctly reflects:
- high attack complexity (race timing)
- privileged trigger
- low confidentiality/integrity impact (read might leak a few bytes of
  freed memory into the user's getsockopt buffer)
- high availability impact (panic)

## Recommended fix

`fix.diff` is a substantial refactor that mirrors the netmsg-dispatch
pattern used by every other cross-CPU function in this file:

1. Add `ip_fw3_ctl_state_get_dispatch(netmsg_t nmsg)` that runs on the
   owning CPU's netisr, walks ONLY the local CPU's six state trees with
   `RB_FOREACH_SAFE`, and writes results into a shared output cursor.
2. Rewrite `ip_fw3_ctl_state_get` to allocate the dispatch context
   (output cursor + remaining-count + overflow flag), `netmsg_init`
   with the dispatch function, `netisr_domsg(&msg, 0)`, then return
   the total bytes written.
3. NULL-guard `s->stub` (defense-in-depth against DF-0631).

The macro-style EMIT keeps the per-tree loop compact and identical to
the original semantics; only the synchronization model changes.

This **supersedes** the finding markdown's high-level proposal ("move
every per-CPU RB traversal behind a netmsg, use RB_FOREACH_SAFE") by
giving the concrete implementation.

## PoC files

- `args_overflow.c` — C documentation stub.
- `df0633_test.sh` — stress-test driver: concurrent state churn +
  state-show loop.
- `build.sh`, `run.sh` — build/run wrappers.
- `run.log` — full stress-test output (no panic observed this run).
- `env.txt` — guest environment.
- `fix.diff` — substantial refactor implementing per-CPU netmsg dispatch.

## Caveats

- The race didn't fire in two stress runs (50 and 500 iterations) on
  6-CPU SMP. The bug is real but non-deterministic. A longer run, or a
  hardened debug build with slab poisoning + tree-rotation assertions,
  would likely catch it.
- The unrelated `assertion: z->z_Mic_SLAB_MAGIC in _slabfree` observed
  earlier on some `ipfw3 state show` invocations is a separate latent
  bug in the same code path; it does not directly confirm DF-0633 but
  does indicate the path has memory-safety issues worth investigating.
- The fix.diff is substantial (119 lines removed, 82 added) and should
  be reviewed carefully before merging. In particular, the dispatch
  writes directly to the user-supplied buffer (`sopt->sopt_val`); this
  is safe because the userland context is blocked in getsockopt, but
  the buffer lifetime is worth double-checking.