DF-0276 — Wrong-pointer kfree in DIOCADDADDR error path (PF ioctl)

Claim

pf_ioctl.c DIOCADDADDR error path (line 2169) does kfree(ap, M_PFPOOLADDRPL) where ap is the framework-owned dev_ioctl_args * parameter of pfioctl(struct dev_ioctl_args *ap) (line 981) — not the freshly kmalloc'd pa. The correct sibling path at line 2178 uses pa. Freeing a non-heap (stack) pointer corrupts slab metadata / panics and also leaks pa. /dev/pf is 0600 root:wheel, so this is root→kernel.

Build

cc -o diocaddaddr_wrong_kfree diocaddaddr_wrong_kfree.c

(self-contained: no kernel headers; ioctl numbers + struct offsets measured on this kernel, sizeof(pfioc_pooladdr)=1136.)

Run (as root — /dev/pf is 0600)

kldload pf
./diocaddaddr_wrong_kfree

Expected — VERIFICATION OUTCOME: NOT REPRODUCED (dead code)

The PoC was built and run as root on the unpatched guest (pf.ko loaded, /dev/pf 0600). Result:

[+] got ticket=2
[+] DIOCADDADDR bogus ifname -> expect kernel panic now
[+] DIOCADDADDR returned rc=0 errno=0 (Undefined error: 0) -- NO panic

No panic, no corruption. The reason, traced line-by-line in the source:

the buggy branch is guarded by if (pa->kif == NULL) at pf_ioctl.c:2168. pa->kif is assigned from pfi_kif_get(pa->ifname) at pf_ioctl.c:2167. pfi_kif_get() (sys/net/pf/pf_if.c:185) is a lookup-or-create function: it calls pfi_kif_find() (an RB_FIND, returns NULL if absent) and, if not found, unconditionally kmallocs a new kif with M_WAITOK and returns it (pf_if.c:195-201). M_WAITOK never returns NULL (it blocks until memory is available, or panics the whole system on catastrophic OOM — but never hands back a NULL pointer). There is no path by which pfi_kif_get() returns NULL in normal operation.

Therefore the if (pa->kif == NULL) test at pf_ioctl.c:2168 is dead code, and the buggy kfree(ap, ...) at line 2169 is unreachable at runtime. The PoC confirms this: pfi_kif_get("zznonexist0") happily creates a kif for the bogus name, pa->kif is non-NULL, the success path runs, and pa is inserted into pf_pabuf (line 2182) — no error path, no wrong-pointer free, no leak.

Verdict

NOT REPRODUCED at runtime — latent (dead-code) defect. The code at line 2169 is genuinely wrong (it would corrupt/panic if reached), but the only caller of its guard (pfi_kif_get) is create-on-demand and never returns NULL, so the branch cannot be entered on this kernel. It would become a live root→kernel corruption only if pfi_kif_get's contract were changed to reject unknown names.

The one-line fix (kfree(ap,...) → kfree(pa,...)) is still correct and worth applying as defense-in-depth / latent-bug hygiene. It is verified to apply cleanly and to compile into pf.ko.

Trigger that was attempted

DIOCBEGINADDRS (get ticket) → DIOCADDADDR with addr.type=PF_ADDR_ADDRMASK, af=AF_INET, ifname="zznonexist0" (a name no interface has), carrying the valid ticket. This reaches pfi_kif_get("zznonexist0"), which creates a kif rather than returning NULL, so the kif==NULL error path is not taken.

DF-0276 — VERDICT

Verdict: NOT REPRODUCED (latent / dead-code defect).

The claim

sys/net/pf/pf_ioctl.c, DIOCADDADDR handler, error path at line 2169:

2160:  pa = kmalloc(sizeof(struct pf_altq), M_PFPOOLADDRPL, M_WAITOK|M_NULLOK);
...
2165:  bcopy(&pp->addr, pa, sizeof(struct pf_pooladdr));
2166:  if (pa->ifname[0]) {
2167:      pa->kif = pfi_kif_get(pa->ifname);
2168:      if (pa->kif == NULL) {
2169:          kfree(ap, M_PFPOOLADDRPL);   /* BUG: frees ap, not pa */
2170:          error = EINVAL;
2171:          break;
2172:      }
2173:      pfi_kif_ref(pa->kif, PFI_KIF_REF_RULE);
2174:  }
...
2178:  kfree(pa, M_PFPOOLADDRPL);           /* correct sibling path */

ap is pfioctl's own parameter (struct dev_ioctl_args *ap, line 981) — a pointer into the device-framework stack, not a heap object. kfree(ap, ...) would corrupt slab metadata / panic and leak pa.

Why it does NOT reproduce — the guard is dead code

The buggy line is inside if (pa->kif == NULL) (line 2168). pa->kif comes from pfi_kif_get(pa->ifname) (line 2167). Tracing pfi_kif_get (sys/net/pf/pf_if.c:185):

185: struct pfi_kif *
186: pfi_kif_get(const char *kif_name)
187: {
188:     struct pfi_kif *kif;
189:     if ((kif = pfi_kif_find(kif_name)))
190:         return (kif);
191:     /* Create a new one */
195:     kif = kmalloc(sizeof(*kif), M_PFI, M_WAITOK | M_ZERO);
196:     strlcpy(kif->pfik_name, kif_name, sizeof(kif->pfik_name));
...
200:     RB_INSERT(pfi_ifhead, &pfi_ifs, kif);
201:     return (kif);
202: }

pfi_kif_find() is a plain RB_FIND (returns NULL if absent). If absent, pfi_kif_get unconditionally allocates a new kif with M_WAITOK and returns it. M_WAITOK blocks until memory is available — it never returns NULL (catastrophic OOM panics the whole system, but does not hand back NULL). There is no code path by which pfi_kif_get() returns NULL in normal operation.

Consequently the if (pa->kif == NULL) test at pf_ioctl.c:2168 is dead code, and the buggy kfree(ap, ...) at line 2169 is unreachable at runtime. This is case (d) of the procedure: genuinely not reachable on this kernel — here the reason is a create-on-demand callee, not an off config.

Runtime confirmation (as root, unpatched #0 guest, pf.ko loaded)

A self-contained PoC (diocaddaddr_wrong_kfree.c, hardcoded ioctl numbers and measured pfioc_pooladdr field offsets) drives exactly the trigger the finding describes — DIOCBEGINADDRS (valid ticket) then DIOCADDADDR with a bogus ifname="zznonexist0":

[+] got ticket=2
[+] DIOCADDADDR bogus ifname -> expect kernel panic now
[+] DIOCADDADDR returned rc=0 errno=0 (Undefined error: 0) -- NO panic

pfi_kif_get("zznonexist0") creates a kif for the unknown name, pa->kif is non-NULL, the success path runs (pa is TAILQ_INSERT_TAIL'd at line 2182), and no error path / wrong-pointer free / leak occurs. Guest stayed up. No panic in boot.log.

Fix

findings/poc/DF-0276/fix.diff changes kfree(ap, ...) → kfree(pa, ...) at line 2169 (the obvious correct typo fix matching the sibling at 2178). Verified git apply --check clean and compiles into pf.ko. Because the buggy line is dead code, runtime before/after validation is not possible (no bad behavior to suppress); fix_status: not_testable (the fix is a trivially-correct latent-defect hygiene change, applies + compiles).

Impact

None at runtime on this kernel (dead code). Classified as a latent code defect worth fixing for defense-in-depth; not a live vulnerability as filed.