DF-1095 — iicsmb_bwrite leaves I2C bus in STARTED state on write failure

Build

cc -O0 -o df1095_harness df1095_harness.c
cc -O0 -DFIX -o df1095_harness_fix df1095_harness.c

Run

./df1095_harness        # bus stuck STARTED, phase 2 returns EINVAL
./df1095_harness_fix    # bus released, phase 2 works
sh verify.sh            # 8 static source checks

Expected (bug present)

• verify.sh reports PASS=8 FAIL=0: the iicsmb_bwrite error: label lacks iicbus_stop, while the sibling iicsmb_bread has it (control case); iicbus_start sets sc->started; iicbus_start rejects with EINVAL when started is already set; iicbus_stop clears it.
• df1095_harness phase 1 leaves started != 0; phase 2 returns EINVAL and prints BUG: bus still STARTED ... permanently wedged.
• df1095_harness_fix releases the bus in phase 1; phase 2 succeeds.

Bug shape

iicsmb_bwrite at sys/bus/iicbus/iicsmb.c:462-482 is the only SMBus method in the file that does not call iicbus_stop() in its error cleanup. After iicbus_start() succeeds (setting sc->started = slave, iiconf.c:178), any failure in iicbus_write() jumps to the error: label at :480-481, which is a bare return (error) — no STOP is issued. sc->started stays set; every subsequent iicbus_start() on that bus returns EINVAL (iiconf.c:174-175). The entire SMBus bridge is therefore permanently wedged for all consumers (/dev/smb, kernel sensors, battery/thermal monitors, IPMI SSIF) until driver reload or reboot.

The sibling iicsmb_bread (:484-507) does call iicbus_stop() in its error: label (:505), as do all other read/write methods in the file. iicsmb_bwrite is the sole outlier.

Impact / preconditions

Two reachable paths:

1. Local root via SMB_BWRITE ioctl to a slave that ACKs the address byte but NACKs the first data byte (perfectly legal SMBus behavior for many slave controllers).
2. Untrusted peripheral — a malicious/untrusted I2C device (USB-C/Thunderbolt dock with embedded I2C controller, BMC, etc.) NACKs mid-transaction.

Either path permanently disables the SMBus bridge — DoS for all I2C/SMBus consumers on that bus.

The audit QEMU guest has no iicbus/iicsmb devices in dmesg, so the path is not exercised dynamically here; the bug is confirmed by source trace + harness.

DF-1095 — iicsmb_bwrite leaves I2C bus in STARTED state on write failure

Verdict

NOT REPRODUCED at runtime (hardware-gated) — STATIC VERIFICATION + HARNESS CONFIRMED.

The bug exists verbatim in sys/bus/iicbus/iicsmb.c:462-482. The iicsmb_bwrite function is the only SMBus method in the file whose error: cleanup label does not call iicbus_stop(). When iicbus_start() succeeds (setting sc->started = slave, iiconf.c:178) and a subsequent iicbus_write() fails (NACK/bus error/timeout), the function returns via the bare error: label at :480-481 without issuing STOP. sc->started stays set; every subsequent iicbus_start() on that bus returns EINVAL (iiconf.c:174-175) until driver reload or reboot. The entire SMBus bridge is therefore permanently wedged for all consumers (/dev/smb, kernel sensors, battery/thermal monitors, IPMI SSIF).

The audit QEMU guest has no iicbus/iicsmb devices in dmesg (no I2C controller in pciconf -l), so the path is not exercised at runtime here. The trigger requires either local root with an exposed /dev/smb whose slave NACKs the data byte, or a malicious/untrusted I2C peripheral. The bug is confirmed by source trace + harness.

The df1095_harness userspace C program mirrors the iicbus state machine (sc->started) and the exact iicsmb_bwrite call graph. In the unpatched mode, phase 1 (start succeeds, write NACKs) leaves started = slave and no stop call is made; phase 2 then sees iicbus_start return EINVAL (rc=22) — the bridge is wedged. In the patched mode (iicbus_stop() in the error label), phase 1 calls stop, and phase 2 succeeds.

Mechanism (confirmed by source trace)

iicbus_start (iiconf.c:169-183):

int
iicbus_start(device_t bus, u_char slave, int timeout)
{
    struct iicbus_softc *sc = ...;
    int error = 0;

    if (sc->started)
        return (EINVAL);                  /* :175 — bus already started */

    if (!(error = IICBUS_START(...)))
        sc->started = slave;              /* :178 — mark bus STARTED */
    else
        sc->started = 0;

    return (error);
}

iicbus_stop (iiconf.c:213-226) clears sc->started = 0 (:224).

iicbus_request_bus and iicbus_release_bus (iiconf.c:86-148) manage only sc->owner, NOT sc->started — confirmed by verify.sh check 8.

iicsmb_bwrite (iicsmb.c:462-482):

if ((error = iicbus_start(parent, slave & ~LSB, IICBUS_TIMEOUT)))
    goto error;                           /* :468-469 — start FAILED, no STOP needed */
if ((error = iicbus_write(parent, &cmd, 1, &sent, IICBUS_TIMEOUT)))
    goto error;                           /* :471-472 — start SUCCEEDED, STOP needed */
if ((error = iicbus_write(parent, buf, (int)count, &sent, IICBUS_TIMEOUT)))
    goto error;                           /* :474-475 — start SUCCEEDED, STOP needed */
if ((error = iicbus_stop(parent)))
    goto error;                           /* :477-478 — redundant */

error:
    return (error);                       /* :480-481 — NO STOP!!! */

The first goto error (start failed) is safe — sc->started was not set. The second and third goto errors (start succeeded, write failed) are buggy — sc->started is still set when the function returns.

Compare iicsmb_bread (:484-507):

if ((error = iicbus_start(...)))
    return (error);                       /* :490-491 — early return */
...
error:
    iicbus_stop(parent);                  /* :505 — ALWAYS stop */
    return (error);                       /* :506 */

That pattern is the canonical correct form. iicsmb_bwrite is the sole outlier — every other read/write function in iicsmb.c calls iicbus_stop() in its error label.

Reproduction

$ sh verify.sh        # 8/8 static checks
$ cc -O0 -o df1095_harness df1095_harness.c
$ cc -O0 -DFIX -o df1095_harness_fix df1095_harness.c
$ ./df1095_harness        # phase 2: rc=22 (EINVAL), started != 0
$ ./df1095_harness_fix    # phase 2: rc=0, started == 0

Fix

fix.diff rewrites iicsmb_bwrite to mirror iicsmb_bread:

1. Change iicbus_start failure from goto error to return error (no STOP needed when start failed — sc->started was not set).
2. Move iicbus_stop() into the error: label so it is always called on the success path AND on the write-failure paths.
3. Remove the redundant explicit iicbus_stop() + goto error at :477-478 (now handled by the error label).

The nativekernel build of the patched file succeeds; the harness validates the algorithm-level correctness.