# DF-0759 — Double NG_FREE_ITEM on ng_split out-hook data path

## Verdict: REPRODUCED (panic on INVARIANTS kernels) — root-only, non-default module, NO memory corruption

The double `NG_FREE_ITEM` described in the finding is **real and reproduced as a
kernel panic (KKASSERT) on INVARIANTS builds**. It is, however, a narrow defect:

- **reachability is root-only** (a netgraph topology — needed to deliver data to
  a split node's `out` hook — can only be set up through the privileged netgraph
  control socket);
- **the module is non-default** — `ng_split` exists only in `sys/netgraph7/`,
  the opt-in parallel netgraph stack; it is neither in `X86_64_GENERIC` nor in
  the default module build (`Makefile.modules` picks `sys/netgraph/` unless
  `WANT_NETGRAPH7` is set, and `ng_split` has no upstream module Makefile /
  SUBDIR entry);
- **the primitive is a flag-double-set caught by an assertion, NOT a memory
  double-free** — `NG_FREE_ITEM` only sets the `NGQF_FREE` "mark for later free"
  flag; the actual `ng_free_item()` runs once, later, in the message-reply path.
  There is **no slab double-free, no UAF, no write primitive** → **no escalation
  path** (Phase 6 does not apply: this is not memory corruption).

## Root cause (confirmed by code + live reproduction)

`sys/netgraph7/ng_split.c` `ng_split_rcvdata()`:

```c
  if (hook == priv->out) {
      kprintf("ng_split: got packet from out hook!\n");
      NG_FREE_ITEM(item);          /* :136  marks NGQF_FREE (does NOT null item) */
      error = EINVAL;
  } else if ((hook == priv->in) && (priv->mixed != NULL)) {
      NG_FWD_ITEM_HOOK(error, item, priv->mixed);   /* nulls item on success */
  } else if ((hook == priv->mixed) && (priv->out != NULL)) {
      NG_FWD_ITEM_HOOK(error, item, priv->out);
  }

  if (item)                        /* :144  item still non-NULL in the out-hook case */
      NG_FREE_ITEM(item);          /* :145  second NG_FREE_ITEM  -> KKASSERT trips */
```

The non-debug `NG_FREE_ITEM` (`sys/netgraph7/netgraph.h:816-820`):

```c
#define NG_FREE_ITEM(item)                       \
    do {                                         \
        KKASSERT(!(item->el_flags & NGQF_FREE)); /* :818 fires on the 2nd call */ \
        item->el_flags |= NGQF_FREE;             /* does NOT null `item` */        \
    } while (0)
```

`KKASSERT` (`sys/sys/systm.h:94-101`) is gated on `options INVARIANTS`, which the
default `X86_64_GENERIC` ships (`sys/config/X86_64_GENERIC:56`). So on an
INVARIANTS build the second `NG_FREE_ITEM` evaluates `!(item->el_flags &
NGQF_FREE)` to FALSE (the flag was already set by the first call at :136),
tripping the assertion → `panic`. On a non-INVARIANTS (production) build the
`KKASSERT` compiles to `do {} while(0)` and the second call is an idempotent
flag-set — harmless.

(The debug `NG_FREE_ITEM`, `netgraph.h:787-792`, *does* null `item`, so under
`NETGRAPH_DEBUG` the `if(item)` guard at :144 prevents the second call — another
reason this only bites the non-debug INVARIANTS path.)

## How it was reproduced

The guest runs the **old** `sys/netgraph/` netgraph (the default); `ng_split` is
absent from both the kernel and `/boot/kernel/`. To exercise the bug the
netgraph7 stack was built from the in-guest `/usr/src`:

1. `netgraph7` core + `ng_socket7` modules built from `sys/netgraph7/netgraph/`
   and `sys/netgraph7/socket/` (stock).
2. `ng_split` built from `sys/netgraph7/ng_split.c` with `KCFLAGS=-DINVARIANTS`
   (a hand-authored module Makefile; `bsd.kmod.mk` does **not** inherit the
   kernel's `INVARIANTS` define, so a stock kld build has `KKASSERT` compiled
   out — this `KCFLAGS` reproduces the INVARIANTS-kernel behaviour the finding
   describes, equivalent to `options NETGRAPH7_SPLIT` in an INVARIANTS kernel).
3. `libnetgraph7` (NG_VERSION=8) built/installed so the userland injector speaks
   the netgraph7 wire ABI (the stock `ngctl`/`libnetgraph` is NG_VERSION=2 and is
   rejected by netgraph7's `ng_socket` at `ng_socket.c:264-268`).
4. The injector (`inject.c`) creates a socket node, peers its `tx` hook to a new
   `split` node's `out` hook, and writes one data byte. The mbuf is delivered to
   `split:out` → `ng_split_rcvdata(hook == priv->out)` → the out-hook branch.

### Observed on the unpatched code (INVARIANTS active)

```
login: ng_split: got packet from out hook!
panic: assertion "!(item->el_flags & NGQF_FREE)" failed in ng_split_rcvdata at /usr/src/sys/netgraph7/split/ng_split.c:145
cpuid = 0
Trace beginning at frame 0xfffff801185299f0
ng_split_rcvdata() at ng_split_rcvdata+0xfd 0xffffffff8260b0fd
ng_split_rcvdata() at ng_split_rcvdata+0xfd 0xffffffff8260b0fd
ng_apply_item() at ng_apply_item+0x105 0xffffffff82601a55
ngthread() at ngthread+0x18 0xffffffff82603488
Debugger("panic")
Stopped at Debugger+0x7c: movb $0,0xbdaf09(%rip)
db>
```

(The trace also confirms the data path the finding describes: the `kprintf` at
:135 fires, then the panic cites :145 — the catch-all second `NG_FREE_ITEM`.)

### Impact / escalation

This is a **KKASSERT panic (local DoS)**, root-only, in a non-default module. It
is **not** memory corruption: `NG_FREE_ITEM` sets a mark-for-later-free flag, the
actual free happens once. There is **no double-free primitive, no UAF, no write**
→ no privilege-escalation chain (Phase 6 is not applicable). On a production
(non-INVARIANTS) kernel the second call is a no-op flag-set and the defect is
silent.

## Fix (validated)

Remove the redundant `NG_FREE_ITEM(item)` from the out-hook branch; the existing
catch-all `if (item) NG_FREE_ITEM(item)` at :144-145 then frees the (un-forwarded)
item exactly once. This matches the finding's recommendation and the
`ng_hub.c:71-72` early-free pattern. See `fix.diff`.

### Before / after (both with `KCFLAGS=-DINVARIANTS`, same trigger)

| build                 | out-hook `kprintf` | result                                              |
|-----------------------|--------------------|-----------------------------------------------------|
| unpatched ng_split    | fires              | `panic: assertion "!(item->el_flags & NGQF_FREE)"` → guest down |
| patched ng_split      | fires              | `NO_PANIC`, guest stays up, no assertion            |

Both runs drove the identical trigger (data byte → `split:out`); the patched
module still reaches the out-hook branch (the `kprintf` fires in `dmesg`) but
frees the item once instead of twice.