DF-4713 — amdgpu_irq.c: unchecked optional-callback calls in dispatch/update

File: sys/dev/drm/amd/amdgpu/amdgpu_irq.c (pass 2, GLM 5.3) Severity: Low · Confidence: likely (missing check certain; trigger config/hardware-gated)

Build / Run

Hardware-gated (requires an AMD GPU for the amdgpu IRQ machinery to run; the audit QEMU guest has only virtio-gpu — verified, see env.txt). No runtime PoC per the DF-4519/DF-2201 HW-gated protocol. The fix is compile-validated against the patched amdgpu_irq.c inside the full amdgpu KLD module build (-Werror):

# guest (root):
cd /usr/src && patch -p1 < /tmp/df4713_fix.diff
cd /usr/src/sys/dev/drm/amd/amdgpu && make    # MOD_BUILD_RC=0 (build.log)

What to look at

• VERDICT.md — full narrative incl. the DF-4643 lock-type verification and all killed candidates (fini double-free, get/put races, registry bounds).
• build.log — full untrimmed amdgpu module build log (MOD_BUILD_RC=0).
• fix.diff — NULL guards for funcs->process (dispatch) and funcs->set (update), matching the file's own existing guard style.
• env.txt — guest uname, no-AMD-GPU proof, shim citations.

DF-4713 VERDICT — amdgpu_irq.c unchecked optional callbacks (.process/.set)

Status: untested (runtime) — the missing checks are source-certain; the NULL-call trigger requires an AMD GPU + dce_virtual display configuration the audit guest cannot provide. Fix compile-validated in the full amdgpu module build (-Werror, MOD_BUILD_RC=0).

Context: DF-4643 lock-type verification (requested by the orchestrator)

DF-4643 (dce_virtual .set → DragonFly hrtimer_cancel may sleep under adev->irq.lock) — the lock's nature verified in THIS file:

• amdgpu_irq.c:237: lockinit(&adev->irq.lock, "agail", 0, LK_CANRECURSE); — DragonFly struct lock (sleepable lockmgr), NOT a raw spinlock.
• spinlock_t is typedef struct lock spinlock_t (sys/dev/drm/include/linux/spinlock_types.h:41), and spin_lock_irqsave(l, f) = local_irq_save(f) + lockmgr(l, LK_EXCLUSIVE) (sys/dev/drm/include/linux/spinlock.h:75-79).
• local_irq_disable() is only crit_enter() — a critical section, not a hard CPU IRQ disable (sys/dev/drm/include/linux/irqflags.h:34-38, with Dillon's comment explaining the deliberate choice).

So the "spinlock" is a sleepable lockmgr + crit section. The DF-4643 hazard (callback sleeping while holding it inside the crit section) stands as filed; that defect lives in the dce_virtual callback + shim, not re-filed here. The LK_CANRECURSE at :237 exists precisely because .set callbacks invoked under irq.lock (amdgpu_irq.c:139, :467) call back into irq code that retakes it — consistent, no distinct defect in the init itself.

The defect (distinct, in THIS file)

The irq registry treats both callbacks as optional and validates them inconsistently:

• amdgpu_irq_add_id (amdgpu_irq.c:359-360) only requires source->funcs != NULL — it happily registers sources with funcs->set == NULL and/or funcs->process == NULL.
• amdgpu_irq_disable_all (:134) checks !src->funcs->set before calling.
• amdgpu_irq_get/amdgpu_irq_put (:520-521, :550-551) check !src->funcs->set before proceeding.
• But amdgpu_irq_dispatch (:436) calls r = src->funcs->process(adev, src, entry); with no NULL check, in hardirq context; and amdgpu_irq_update (:467) calls r = src->funcs->set(adev, src, type, state); with no NULL check.

In-tree NULL providers exist:

• dce_virtual_crtc_irq_funcs = { .set = dce_virtual_set_crtc_irq_state, .process = NULL } (dce_virtual.c:757-760), registered at dce_virtual.c:375 on client LEGACY src_id VISLANDS30_IV_SRCID_SMU_DISP_TIMER2_TRIGGER (=229, ivsrcid/ivsrcid_vislands30.h:267) with crtc_irq.num_types = AMDGPU_CRTC_IRQ_VBLANK6 + 1 (=7 > 0, dce_virtual.c:762). If the IH ring ever delivers a LEGACY/229 vector on a dce_virtual system (amdgpu.virtual_display=1 on ASICs whose firmware/ display block still raises DISP_TIMER2, or a stray/stale IV after GPU reset), amdgpu_irq_dispatch performs a NULL function-pointer call in hardirq → immediate kernel panic. Impact ceiling: local DoS.
• The sdma_v*_illegal_inst_irq_funcs (.process only, no .set — e.g. sdma_v4_0.c:1748-1750) currently carry num_types == 0, so amdgpu_irq_gpu_reset_resume_helper's k < src->num_types loop (:493) never reaches amdgpu_irq_update for them — but the helper (:488-494) performs no .set check of its own, so one future registration with num_types > 0 and no .set turns every GPU reset (user-triggerable by hanging the GPU from an unprivileged DRI client) into a NULL-call panic on the recovery path (amdgpu_device.c:3287,3341).

The code already knows the callbacks are optional (three sibling call sites check); dispatch and update are the two holes.

Why Low

• No demonstrated HW path delivers src 229 to dispatch under dce_virtual on this guest (no AMD GPU at all — pciconf shows only virtio-gpu 1af4:1234).
• The update hole is not reachable with the current registry (all num_types > 0 sources define .set — verified across all 61 amdgpu_irq_add_id call sites).
• Defense-in-depth fix is two trivial guards; matches upstream's own convention elsewhere in this file.

Killed candidates (negative results, this file)

• add/remove registry bounds: add_id bounds-checks client_id/src_id (:353-357), rejects dup slots (:371-372); dispatch bounds-checks both ids before indexing (:409-417). No remove API exists in this snapshot. Clean.
• num_types/enabled_types sizing: every IP sets num_types in early_init (e.g. sdma_v4_0.c:1276-1288) before sw_init's add_id (:1301+), so enabled_types is always kcalloc'd with the final count (:374-383); get/put/update/enabled all re-check type >= num_types (:517,:547,:578). Clean.
• fini double-free: the double-registered static sources (gmc_v9_0.c:963/965 vm_fault on VMC+UTCL2; sdma_v4_0.c:1301/1307 trap_irq on SDMA0+SDMA1) all have data == NULL, so the kfree(src) branch (:325-329) never fires for them; enabled_types is freed once then NULL'd (second iteration kfree(NULL) — safe); the sources array itself is freed and NULL'd (:331-332) leaving no dangling registry pointer. Clean.
• dispatch (hardirq) vs fini UAF: amdgpu_irq_fini first calls drm_irq_uninstall (:302) → free_irq (linux_irq.c:108-141) → bus_teardown_intr, which unregisters and synchronizes the handler before the kfree loop (:313-333) runs. Clean at teardown.
• get/put refcount protocol: the re-check of amdgpu_irq_enabled() under the lock inside amdgpu_irq_update (:462-467) makes concurrent get/put converge to the correct HW state; no underflow path reachable without a pre-existing driver bug (unbalanced put).
• in-flight get/put vs fini kfree(enabled_types): theoretical window (irq_enabled cleared at drm_irq.c:183 before teardown completes); requires privileged device teardown racing an ioctl — upstream-identical; noted, not filed.
• virq[src_id] empty-then (:419-423): amdgpu_irq_create_mapping returns 0 with its body #if 0'd (:692-699), so virq[] is never set and the (empty) then-branch is dead. Benign.
• hotplug work (:80-94): connector walk under mode_config->mutex; hotplug_work flushed in fini (:309) before connector teardown. Clean.
• irq_callback ring_index (:162-172): bounded by ih->rptr maintenance in amdgpu_ih.c (out of scope for this file). Clean.
• MSI/MSI-X: DragonFly branch reads _irq_type (:243); no vector-count math exists here to get wrong. Clean.

Recommended fix

fix.diff (this pack): NULL-guard funcs->process in dispatch and funcs->set in update (releasing the lock before returning -EINVAL in the update path), matching the guards disable_all/get/put already use.

Fix status (batch validation)

Applied with the 36 sibling fix.diffs of this batch in findings/poc/_batch41/all_fixes.patch; single in-guest make -j6 nativekernel KERNCONF=X86_64_GENERIC completed RC=0 with zero errors under -Werror (full log: findings/poc/_batch41/fix_build8.log). Behavior A/B intentionally skipped for this Info-tier item.