DF-4595 — read_drive_label() ignores the read_drive() error and decides on stale/uninitialized heap

• File: sys/dev/raid/vinum/vinumio.c:336-337
• Severity: Low · Confidence: certain (missing check) / likely (impact)

What

vhdr = (struct vinum_hdr *) Malloc(VINUMHEADERLEN);   /* NOT zeroed */
...
drive->state = drive_up;                              /* be optimistic */
error = read_drive(drive, (void *) vhdr, VINUMHEADERLEN, VINUM_LABEL_OFFSET);
if (vhdr->magic == VINUM_MAGIC) {                     /* error never tested */

Malloc is plain kmalloc(M_DEVBUF, …) (vinumext.h:258) — no M_ZERO. If read_drive() fails (media error, device yanked, I/O fault), vhdr holds whatever the heap had. Because the previous call in the same scandisk loop does Malloc(VINUMHEADERLEN) … Free(vhdr) of the same size class, a failing read very likely resurrects the previous partition's valid header (same 512-byte block reused), so vhdr->magic == VINUM_MAGIC still holds and the failed drive is classified DL_OURS with the previous drive's label: drive->label = vhdr->label copies the stale identity, check_drive() then merges the device into the configuration under a wrong name.

Impact

• Wrong-drive acceptance on read error → the config may bind a different device to a drive name; subsequent I/O for that drive's subdisks goes to the device that failed (or nothing at all if open fails later). On a mirrored setup this is a data-integrity hazard, not a memory-safety one.
• If the recycled buffer contains no NUL in label.name-adjacent bytes, the strcmp(drive->label.name, vhdr->label.name) at line 339 walks beyond the 512-byte allocation (OOB read; both operands are in-struct here so it usually terminates, but it is unbounded in principle).

Why untested (no guest run)

Forcing a failing sector read at exactly VINUM_LABEL_OFFSET of an otherwise valid ≥1 MiB vinum partition requires either real failing media or emulator-level fault injection (QEMU blkdebug on the nata path); not feasible on the standard guest snapshot within this run. The missing if (error) is however directly visible in the source and trivially fixed.

Fix

     error = read_drive(drive, (void *) vhdr, VINUMHEADERLEN, VINUM_LABEL_OFFSET);
-    if (vhdr->magic == VINUM_MAGIC) {
+    if (error == 0 && vhdr->magic == VINUM_MAGIC) {

(and let the error fall through to the DL_CANT_OPEN/DL_NOT_OURS handling in check_drive()).

DF-4595 — VERDICT

Status: untested (code-certain defect; impact argument, no guest run — see README "Why untested").

Analysis

• sys/dev/raid/vinum/vinumio.c:332 — vhdr = Malloc(VINUMHEADERLEN): Malloc → kmalloc(…, M_DEVBUF, M_WAITOK) (vinumext.h:258-259), not zeroed.
• sys/dev/raid/vinum/vinumio.c:336 — error = read_drive(...): driveio() (vinumio.c:272-304) leaves buf untouched/partial on error and returns the error.
• sys/dev/raid/vinum/vinumio.c:337 — if (vhdr->magic == VINUM_MAGIC): the error is never consulted before interpreting the buffer. The error variable is dead until read_drive_label() returns and check_drive() looks at drive->lasterror — which the success-shaped DL_OURS result never set.

Reachability: read_drive_label() is called from check_drive() (vinumio.c:376), which vinum_scandisk() calls for every partition of every scanned device, and from config_drive() (vinumconfig.c:1001). A read error at offset 4096 of an opened vinum-typed partition is the trigger; the allocator-reuse argument (same-size Malloc/Free in the scan loop) makes the stale-magic case realistic rather than theoretical.

Classification: a correctness bug with data-integrity consequences and a possible unbounded strcmp read; root/attacker-media adjacent; Low.

Not run on the guest

Requires fault-injected media at a fixed offset (blkdebug-style) — outside this snapshot's capability; recorded as untested rather than guessed.

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.)