# DF-0596 — VERDICT: Unsynchronized SMP race on xmitWin in ng_pptpgre

## Verdict: REPRODUCED (code-level) — TOCTOU race confirmed; OOB write contingent on codegen

**Finding:** The legacy `ng_pptpgre` netgraph node has **no per-node serialization** (no
spinlock, token, or mutex) protecting its mutable state (`xmitWin`, `timeSent[]`,
`winAck`, `recvAck`, `rtt`, `dev`, `ato`, `recvSeq`, `xmitSeq`). On an SMP system, two
GRE packets can be processed concurrently on different CPUs, each invoking
`ng_pptpgre_recv` on the same node without synchronization.

## Root Cause Confirmation (source trace)

1. **No serialization** — `ng_pptpgre_recv` (sys/netgraph/pptpgre/ng_pptpgre.c:566)
   accesses all shared state without any lock. `ng_send_data`
   (sys/netgraph/netgraph/ng_base.c:1678) dispatches `rcvdata` inline on the caller's
   CPU. The ksocket upcall uses `crit_enter()` which is per-CPU only. **Confirmed: no
   serialization exists.**

2. **TOCTOU on xmitWin growth** (ng_pptpgre.c:672-676):
   ```c
   if (PPTP_SEQ_DIFF(ack, a->winAck) >= 0
       && a->xmitWin < PPTP_XMIT_WIN) {     // CHECK (read)
       a->xmitWin++;                          // INCREMENT (non-atomic RMW)
       a->winAck = ack + a->xmitWin;
   }
   ```
   `xmitWin` is `u_int16_t` at struct offset 0x2c in `ng_pptpgre_ackp`
   (ng_pptpgre.c:146). The read-check-increment is non-atomic. Two concurrent ack
   handlers can both see `xmitWin == 15`, both pass `< 16`, and both increment.

3. **OOB write target** (ng_pptpgre.c:514):
   ```c
   a->timeSent[priv->xmitSeq - priv->recvAck] = ng_pptpgre_time(node);
   ```
   The index is `xmitSeq - recvAck`, bounded by `xmitWin`. With `xmitWin > 16`, the
   index can reach 16, one past the end of `pptptime_t timeSent[PPTP_XMIT_WIN]` (16
   elements, valid indices 0..15). `timeSent` is the last field of
   `struct ng_pptpgre_ackp` (ng_pptpgre.c:152), followed immediately by `recvSeq` in
   `struct ng_pptpgre_private` (ng_pptpgre.c:165).

4. **Struct layout verified** — harness confirms `&timeSent[16]` and `&recvSeq` are
   both at offset 200 in `struct ng_pptpgre_private`. An 8-byte write at `timeSent[16]`
   would overwrite `recvSeq` (4 bytes) and `xmitSeq` (4 bytes).

## Harness Results

### Baseline — TOCTOU is winnable
```
=== -O0 (separate load/add/store codegen — maximizes race window) ===
[threads=2] xmitWin>16 in 31 rounds, max_xw=17
[threads=4] xmitWin>16 in 70 rounds, max_xw=17
Total over-16 rounds: 104/30000, max_xw=17
*** CONFIRMED: xmitWin can exceed PPTP_XMIT_WIN -> timeSent[] OOB ***
```

### Baseline — default kernel codegen (-O2)
```
=== -O2 (single load reused for check+increment — compiler prevents exceeding 16) ===
[threads=2/4/8] xmitWin>16 in 0 rounds, max_xw=16
Race not won (xmitWin stayed <= 16). Codegen prevents exceeding the bound.
```

### Kernel disassembly (unpatched module, -O2)
The compiler generates a single load for both check and increment:
```asm
1179: movzwl 0x2c(%rbx),%eax   ; LOAD xmitWin once
117d: cmp    $0xf,%ax           ; CHECK (reuses register)
1183: add    $0x1,%eax          ; INCREMENT (reuses register, NOT reload)
1186: mov    %ax,0x2c(%rbx)     ; STORE
```
With this codegen, two concurrent threads both compute 15+1=16 from their stale
register copy. **xmitWin cannot reach 17 with -O2.**

## Impact Assessment

### What IS confirmed:
- **Missing serialization is a genuine bug** — all shared state in ng_pptpgre_ackp and
  ng_pptpgre_private is accessed without any lock from concurrent CPU contexts.
- **TOCTOU on xmitWin is real** — the read-check-increment race is winnable (proven
  with -O0 harness: 104/30000 rounds reaching xmitWin=17).
- **Struct layout confirms OOB target** — timeSent[16] overlaps recvSeq/xmitSeq at
  offset 200 in the priv struct.
- **Other unsynchronized data races are real and dangerous:**
  - `bcopy(a->timeSent + index + 1, a->timeSent, ...)` at line 668 — two concurrent
    overlapping bcopy operations on the same buffer is undefined behavior.
  - `priv->recvAck = ack` at line 652 — last writer wins.
  - `a->rtt += ...` at line 657, `a->dev += ...` at line 660 — lost updates.
  - `priv->recvSeq = seq` at line 698 — last writer wins.

### What is NOT confirmed on the default kernel:
- The specific heap OOB write at `timeSent[16]` is **not achievable with the default
  -O2 kernel build**. The compiler reuses the register value from the check, so xmitWin
  stays at 16 (valid maximum). The OOB IS achievable with -O0 codegen (harness proof),
  demonstrating the code is inherently unsafe.

### Realistic impact:
- **PPTP session DoS** from corrupted state (recvSeq, rtt, ato, timeSent bcopy races)
  — severity Medium (deprecated protocol, legacy systems only).
- **Heap OOB write** is latent — not reachable with current -O2 codegen, but any
  compiler change, LTO configuration, or future code modification could expose it.
  The bounds check fix is appropriate defense-in-depth.

## PoC Changes

- **Rewrote `race.c`** → `race_harness.c`: The original PoC was a Linux raw-socket
  sketch that couldn't run on the guest (no PPTP concentrator, no external GRE path).
  Replaced with a deterministic pthread-based harness that replicates the exact kernel
  growth logic, struct layout, and race conditions. The harness proves:
  1. Struct layout: timeSent[16] overlaps recvSeq (OOB target).
  2. TOCTOU is winnable with -O0 (xmitWin reaches 17 in ~0.3% of rounds).
  3. TOCTOU is NOT winnable with -O2 (compiler prevents exceeding 16).
- **Added `race_harness_fixed.c`**: Same harness with the fix applied (clamp + bounds
  check), showing zero OOB even with -O0 codegen.

## Fix (fix.diff)

Two-part defense-in-depth fix:

1. **Bounds check on timeSent index** (ng_pptpgre.c:514): Wrap the array access in an
   explicit bounds check:
   ```c
   u_int32_t _ts_idx = priv->xmitSeq - priv->recvAck;
   if (_ts_idx < PPTP_XMIT_WIN)
       a->timeSent[_ts_idx] = ng_pptpgre_time(node);
   ```
   This prevents the OOB write regardless of what xmitWin does.

2. **Defensive clamp on xmitWin** (ng_pptpgre.c:674): After the increment, clamp to
   PPTP_XMIT_WIN:
   ```c
   if (a->xmitWin > PPTP_XMIT_WIN)
       a->xmitWin = PPTP_XMIT_WIN;
   ```
   (Note: gcc -O2 optimizes this away as dead code since the preceding check already
   prevents exceeding 16; it's included for defense-in-depth and future-proofing.)

A more complete fix would add a per-node spinlock to serialize all ack-processing
state modifications (the finding markdown's recommendation), but this is complex due
to reentrancy (ng_pptpgre_xmit is called from ng_pptpgre_recv). The bounds check +
clamp is the minimal targeted fix that eliminates the specific OOB write vulnerability.

## Fix Validation

| Test | Result |
|------|--------|
| fix.diff applies cleanly | `patch -p1 --dry-run` rc=0, both hunks succeeded |
| Kernel builds with fix | `make -j6 nativekernel` rc=0 |
| Fixed module has bounds check | Disassembly: `cmp $0xf,%r15d; jbe 970` at ng_pptpgre_xmit+0x6d |
| Harness -O0 baseline (unfixed) | 104/30000 rounds: xmitWin=17 (OOB achievable) |
| Harness -O0 with fix | 0/30000 rounds: xmitWin<=16 (no OOB) |
| Patched kernel boots | `6.5-DEVELOPMENT #1: Wed Jul 8 17:57:09 UTC 2026` |
| Module loads on patched kernel | `kldload ng_pptpgre` rc=0 |

**Fix status: FIXED** — the bounds check prevents the OOB write even under adversarial
codegen conditions (-O0). The defense-in-depth clamp prevents xmitWin from exceeding
PPTP_XMIT_WIN.