vop_nremove quota-accounting glue: latent NULL-deref on nc_vp, nlink TOCTOU, wrong mount for PFS overlays

Summary

The hand-written quota-accounting glue in vop_nremove() has three defects: (1) it unconditionally dereferences nch->ncp->nc_vp for VOP_GETATTR with no NULL guard, unlike journal_nremove() which defensively checks the same field; (2) it reads va via an unlocked VOP_GETATTR before acquiring VFS_MPLOCK and then uses the stale va.va_nlink to decide whether to refund space — a TOCTOU that can double-count or miss-count on racing hardlink unlinks; (3) it accounts against nch->mount instead of vq_vptomp(nc_vp) (as vop_write does), mis-accounting on PFS/nullfs overlays where nch->mount differs from the underlying real mount.

No demonstrated memory-safety trigger from current callers (the syscall paths all pass a positive/resolved ncp), so this is recorded as a correctness / hardening finding (Info).

Root cause

sys/kern/vfs_vopops.c:1660-1668:

if ((error = VOP_GETATTR(nch->ncp->nc_vp, &va)) != 0)   /* :1660 no NULL check; unlocked */
    return (error);

VFS_MPLOCK(dvp->v_mount);                                 /* :1663 lock taken AFTER getattr */
DO_OPS(ops, error, &ap, vop_nremove);
/* Only update space counters if this is the last hard link */
if ((error == 0) && (va.va_nlink == 1)) {
    VFS_ACCOUNT(nch->mount, va.va_uid, va.va_gid, -va.va_size);  /* :1667 wrong mount */
}

1. Latent NULL-deref (:1660): nch->ncp->nc_vp is dereferenced with no NULL check. journal_nremove (sys/kern/vfs_jops.c:1218) defensively guards ap->a_nch->ncp->nc_vp; this path does not. Current syscall callers (kern_unlink, kern_rename, nfs_serv.c) always pass a positive ncp via nlookup, so it is not triggered today, but it is a latent panic if any caller passes a negative/invalidate ncp (e.g. under future refactoring or an odd NFS path).
2. TOCTOU on va_nlink (:1660 then :1663): VOP_GETATTR runs unlocked, before VFS_MPLOCK. Two threads unlinking two hardlinks of the same inode can both snapshot va_nlink == 1 (or both == 2), causing the space refund to fire twice (or not at all) — quota-accounting drift.
3. Wrong mount on overlays (:1667): VFS_ACCOUNT(nch->mount, ...) uses the namecache-handle's mount, which on a PFS/nullfs overlay is the overlay mp, while the file's bytes live on the underlying mp. vop_write (:487) correctly uses vq_vptomp(vp). On a stacked mount the refund is applied to the overlay's counters, which may be unlimited or differently quota'd, enabling accounting bypass.

Threat model & preconditions

• Attacker position: local user on a system with vfs_quota_enabled and quotas configured (and, for sub-defect 3, a PFS/nullfs overlay mount).
• Privileges gained or impact: no memory corruption. (1) latent kernel NULL-deref panic (not triggered by current callers); (2)/(3) quota- accounting correctness defects — accounting drift / possible slow quota leakage in the over-refund direction, and accounting bypass on stacked mounts.
• Required config or capabilities: quotas enabled; for (3) a stacked mount the user can read/write through.
• Reachability: unlink(2) / rename(2) on the affected setups.

Proof of concept

No clean unprivileged crash trigger from current callers (hence Info). The accounting defects are reproducible conceptually:

• Quota drift (TOCTOU): create file F (size S), hardlink F2; race two threads unlinking F and F2. If both VOP_GETATTR snapshots read va_nlink == 1, VFS_ACCOUNT refunds S twice, reducing the user's accounted bytes by 2*S for a single S-byte file.
• PFS bypass: mount -t nullfs /real /overlay; write S bytes via /overlay/file, then unlink it. vop_nremove refunds S to /overlay's mnt_acct instead of /real's, so /real's quota counter never decreases.

Impact

Quota-accounting correctness (and a latent NULL-deref that becomes a DoS if a future caller passes a negative ncp). No demonstrated privilege escalation or memory corruption from current callers; recorded as Info.

Recommended fix

Guard nc_vp, take VFS_MPLOCK before VOP_GETATTR, and account against the file's real mount (vq_vptomp(vp)) — matching vop_write:

--- a/sys/kern/vfs_vopops.c
+++ b/sys/kern/vfs_vopops.c
@@ -1649,6 +1649,7 @@ int
 vop_nremove(struct vop_ops *ops, struct nchandle *nch, struct vnode *dvp,
        struct ucred *cred)
 {
    struct vop_nremove_args ap;
    VFS_MPLOCK_DECLARE;
    int error;
    struct vattr va;
+   struct vnode *vp;

    ap.a_head.a_desc = &vop_nremove_desc;
    ap.a_head.a_ops = ops;
@@ -1658,10 +1659,15 @@
    ap.a_dvp = dvp;
    ap.a_cred = cred;

-   if ((error = VOP_GETATTR(nch->ncp->nc_vp, &va)) != 0)
-       return (error);
-
-   VFS_MPLOCK(dvp->v_mount);
+   vp = nch->ncp->nc_vp;
+   if (vp == NULL)
+       return (ENOENT);
+
+   VFS_MPLOCK(dvp->v_mount);
+   if ((error = VOP_GETATTR(vp, &va)) != 0)
+       goto done;
    DO_OPS(ops, error, &ap, vop_nremove);
    /* Only update space counters if this is the last hard link */
    if ((error == 0) && (va.va_nlink == 1)) {
-       VFS_ACCOUNT(nch->mount, va.va_uid, va.va_gid, -va.va_size);
+       VFS_ACCOUNT(vq_vptomp(vp), va.va_uid, va.va_gid, -va.va_size);
    }
+done:
    VFS_MPUNLOCK();

(The goto done label releases VFS_MPUNLOCK correctly; confirm against the file's exact cleanup convention.)

References

• sys/kern/vfs_vopops.c:1660 — unchecked nc_vp deref + unlocked VOP_GETATTR.
• sys/kern/vfs_vopops.c:1667 — VFS_ACCOUNT(nch->mount, ...) (wrong mount).
• sys/kern/vfs_vopops.c:487 — vop_write uses vq_vptomp(vp) (the correct pattern).
• sys/kern/vfs_jops.c:1218 — journal_nremove guards nc_vp defensively.
• CWE-476, CWE-367, CWE-682.

Timeline

• 2026-06-29 Discovered during automated file-by-file audit of sys/kern/vfs_vopops.c.
• pending Reported to DragonFlyBSD security contact.