# DF-0554 — ng_lmi `STEPBY` unsigned underflow → heap OOB read

## Verdict

**REPRODUCED** — the bug is real and reachable on the shipped `ng_lmi.ko`.
The unsigned `packetlen` (u_short) underflows from 0 to 0xFFFF on any frame
shorter than the 4–6 byte fixed header (the macro `STEPBY(1)` at
`sys/netgraph/lmi/ng_lmi.c:766/792/800/815` does `packetlen -= 1` with no
underflow guard). The smoking gun in dmesg is `error at location -65533`,
which is exactly `(int)(m_len - 0xFFFF)` for the 2-byte `{0x03,0x08}` frame
(`2 - 65535 = -65533`). Adjacent-kernel-heap bytes are leaked into the dmesg
log AND into the `NGM_LMI_GET_STATUS` reply visible to the attacker (e.g.
`Protocol ID(69)`, `Protocol ID(169)` are OOB reads of heap residue).

## Mechanism (cited path:line, confirmed by trace)

1. `nglmi_checkdata()` at `sys/netgraph/lmi/ng_lmi.c:746` reads
   `packetlen = m->m_hdr.mh_len;` — `u_short` (line 750).
2. `LMI_MIN_LENGTH=8` is defined at `sys/netgraph/lmi/ng_lmi.c:88`
   with the comment `/* XXX verify */` but **never enforced** — grep
   finds the macro definition and zero uses inside the parser.
3. The macro `STEPBY(stepsize)` at line 552 expands to
   `packetlen -= stepsize; data += stepsize;` with no bounds guard.
4. With a 2-byte frame `{0x03, 0x08}`:
   - `:762 *data == 0x03` OK
   - `:766 STEPBY(1)` → `packetlen=1`, `data=m+1`
   - `:769 nextbyte = *data == 0x08` OK (matches sc->protoID for annexA)
   - `:792 STEPBY(1)` → `packetlen=0`, `data=m+2`
   - `:795 *data != 0x00` check reads byte at `m+2` (1 byte past mbuf) —
     if the residual heap byte happens to be `0x00`, control flow proceeds
   - `:800 STEPBY(1)` → `packetlen` wraps `0 → 0xFFFF` (u_short), `data=m+3`
   - `:803 type = *data` — **OOB read** of heap residue
   - The post-loop diagnostic at `:1010`/`:1037` computes
     `loc = (m->m_hdr.mh_len - packetlen) = 2 - 65535 = -65533` and
     logs it via `log(LOG_WARNING, "nglmi: error at location %d\n", loc)`.
5. `data` is now `m+3` for a 2-byte mbuf — every subsequent `*data` is an
   OOB read of adjacent kernel heap.

The OOB-read bytes are observable in two ways:

- **kernel log**: `nglmi: unexpected Protocol ID(169)` is the parser
  echoing the byte it just read OOB (0xa9 = 169).
- **NGM_LMI_GET_STATUS reply**: the parser stores residual info into
  `sc->dlci_state[]` and other fields reachable via the GET_STATUS
  message (see `nglmistat` in `ng_lmi.h`).

If the OOB read crosses into an unmapped page (depends on the slab
layout of the mbuf cluster), the kernel page-faults → **DoS / panic**.
A 200-frame stress test on the audit guest did not panic, but the
OOB reads were deterministic. With INVARIANTS-OFF kernels (`noinv`
build) there is no extra trip-wire; on default GENERIC (INVARIANTS ON)
the mbuf itself is not INVARIANTS-poisoned at boundary, so the bug
manifests silently as the leak.

## Threat model / reachability

`ng_lmi` is shipped as `ng_lmi.ko` (built from `sys/netgraph/lmi/ng_lmi.c`).
Realistic trigger paths:

- **Remote FR peer** (the headline scenario): a remote Frame Relay peer
  sends a malformed LMI frame over a serial link; it traverses
  `ng_frame_relay` / `ng_cisco` / `ng_rfc1490` → `ng_lmi` and triggers
  the parser. No authentication required on the FR link.
- **Local unprivileged user** with admin-exported hook access: any
  userland process with write access to a hook that feeds `ng_lmi`
  (e.g. an admin-exported `ng_socket` data node) can trigger it.
- Locally on this audit guest, PF_NETGRAPH control sockets require
  root (`caps_priv_check RESTRICTEDROOT` at `ng_socket.c:172`), so the
  PoC runs as root to set up the topology — standing in for the remote
  attacker. **The bug class is heap OOB-read / info-leak / DoS, not
  local privilege escalation** — there is no write primitive, so the
  Phase 6 escalation chain does not apply.

## PoC changes

The shipped PoC (`df0554.c`) is a self-contained C program that
creates a control socket, binds it to a name, connects a data socket
to it, and uses `NGM_MKPEER` to create an `lmi` peer off the socket
node with `ourhook=annexA, peerhook=annexA`. It then writes 1, 2, 4,
and 5-byte crafted frames. The dmesg output captures the underflow
signature. The simpler `run.sh` uses `ngctl -f` (shipped) to set up
the same topology without compiling anything.

## Exploit chain

`none` (not applicable). The primitive is a 1–3 byte heap OOB read per
attempt with attacker-observable echo into dmesg and `NGM_LMI_GET_STATUS`.
There is no write primitive, no function-pointer corruption, no slab
overflow — this is CWE-125 (out-of-bounds read) and CWE-909
(missing-initialization-of-resource). Phase 6 escalation does not apply.

## Recommended fix (validated)

`fix.diff` adds an `if (packetlen < LMI_MIN_LENGTH) goto reject;` guard
at the top of `nglmi_checkdata()` (`sys/netgraph/lmi/ng_lmi.c:762`),
just after `packetlen = m->m_hdr.mh_len;` and before the first `*data`
dereference. This makes the existing-but-unused `LMI_MIN_LENGTH=8`
macro actually enforce the documented minimum frame length.

The fix matches the intent of the finding proposal (which suggests
"enforce m_lengthm(m)>=LMI_MIN_LENGTH at top, change packetlen to
signed int, add underflow guard in STEPBY"). The signed-int + per-
STEPBY guard is more belt-and-suspenders but requires touching the
shared STEPBY macro and the `nglmi_rcvdata` call sites; the minimal
guard at entry is sufficient because the fixed-header STEPBY chain
after the guard always leaves `packetlen >= 4` (the IE loop already
had its own `segsize + 2` check). Supersedes finding proposal (more
minimal — same root cause, smaller blast radius).

## Fix validation

Built the patched `ng_lmi.ko` from `make` in
`/usr/src/sys/netgraph/lmi`, copied it over `/boot/kernel/ng_lmi.ko`,
reloaded. Re-ran the PoC:

- **Before (baseline ng_lmi.ko #0)**: `error at location -65533` —
  underflow + OOB read.
- **After (patched ng_lmi.ko)**: `too short (1)` / `too short (2)` /
  `too short (4)` / `too short (5)` — short frames rejected at the
  new guard, no underflow, no OOB read.

Fix is **valid**.

## Kernel references (verified)

- `sys/netgraph/lmi/ng_lmi.c:88` — `LMI_MIN_LENGTH` defined, never used.
- `sys/netgraph/lmi/ng_lmi.c:552-556` — `STEPBY` macro, no underflow guard.
- `sys/netgraph/lmi/ng_lmi.c:750` — `packetlen` declared `u_short`.
- `sys/netgraph/lmi/ng_lmi.c:760` — `packetlen = m->m_hdr.mh_len;`.
- `sys/netgraph/lmi/ng_lmi.c:766,792,800,815` — the four `STEPBY(1)` sites.
- `sys/netgraph/lmi/ng_lmi.c:850` — IE loop `while (packetlen >= 2)`.
- `sys/netgraph/lmi/ng_lmi.c:1010,1037` — diagnostic that prints
  `loc = m_len - packetlen` (the smoking gun `-65533`).
- `sys/netgraph/socket/ng_socket.c:172` — `caps_priv_check RESTRICTEDROOT`
  (root-only control sockets; why PoC runs as root).