From fa8913800ce8beb2be562480f3c58f2519b7e0f9 Mon Sep 17 00:00:00 2001 From: Matthew Jackson Date: Mon, 22 Jun 2026 15:50:22 -0700 Subject: [PATCH] Fix 3 Opus-audit findings: patch transport-abort, patch AACS align, reset IOCTL - patch (Pass N) now aborts immediately on transport failure (status=0xFF), symmetric with the sweep and single-pass mux. Previously a USB-bridge crash was treated as an ordinary bad sector and the pass hammered the crashed device sector-by-sector until the per-range watchdog expired. (medium) - patch AACS recovery reads are now unit-aligned: a mid-unit single-sector read on an AACS disc was rejected by the decrypting reader (DecryptFailed) and the sector abandoned without asking the drive. The read is now widened to the enclosing whole 3-sector unit and the requested window copied out, leaving all recovery accounting (pos/block_bytes/cursor) untouched so it cannot desync. Only affected CLI decrypt-to-ISO --multipass re-runs. (low) - IOCTL_STORAGE_RESET_DEVICE corrected 0x002D1004 -> 0x002DD000 (the old value decoded to function 0x401 with the access bits cleared, so DeviceIoControl would fail ERROR_INVALID_FUNCTION instead of resetting). Windows-only. (low) Adds a transport-failure classification regression test. --- src/disc/patch.rs | 111 +++++++++++++++++++++++++++++++++++++++++++- src/scsi/windows.rs | 8 +++- 2 files changed, 116 insertions(+), 3 deletions(-) diff --git a/src/disc/patch.rs b/src/disc/patch.rs index bb858a3..f846a7e 100644 --- a/src/disc/patch.rs +++ b/src/disc/patch.rs @@ -926,6 +926,26 @@ pub(super) fn handle_read_failure( shared: &Mutex, reader: &mut R, ) -> Result { + // Transport failure (status=0xFF: USB-bridge crash / disconnect) is not a + // recoverable bad sector — the bridge is wedged and every further read fails + // identically. Abort the pass immediately (symmetric with the sweep's + // read_error::handle_read_error AbortPass and single-pass mux's fill_extents), + // so autorip can drop and re-enumerate the bridge instead of hammering a + // crashed device sector-by-sector until the per-range watchdog expires. + // Checked before the batch-split below: a 0xFF on a batch read is still a + // bridge crash, not an ambiguous bad sector. + if err.is_scsi_transport_failure() { + tracing::warn!( + target: "freemkv::disc", + phase = "patch_transport_failure", + lba, + count, + "transport failure (bridge crash) during patch — aborting pass" + ); + state.wedged_exit = true; + return Ok(FailureAction::BreakOuter); + } + // Adaptive batching split decision: a batch-read failure // (count > 1) is NOT a recorded failure. We don't yet know which // sector in the batch was actually bad — could be one, could be @@ -1678,6 +1698,12 @@ impl Disc { // inline decrypt_sectors call sites that all keyed off the // same `keys`. `DecryptKeys::None` keeps the unencrypted / // --raw path a pass-through. + // AACS reads must start on a 3-sector unit boundary and span whole + // units (DecryptingSectorSource rejects mid-unit reads as DecryptFailed). + // The patch cursor derives from arbitrary mapfile byte offsets, so a + // single-sector recovery read can land mid-unit — see the aligned read + // at the read call site below. + let decrypt_is_aacs = matches!(keys, crate::decrypt::DecryptKeys::Aacs { .. }); let mut reader = DecryptingSectorSource::new(reader, keys); let reader = &mut reader; @@ -1887,8 +1913,38 @@ impl Disc { // sr_mod driver run its own auto-retries (which don't // pay per-attempt escalation in the same way). let read_start = std::time::Instant::now(); - let read_result = - reader.read_sectors(lba, count, &mut buf[..bytes], state.recovery); + let read_result = if decrypt_is_aacs && (lba % 3 != 0 || count % 3 != 0) { + // Mid-unit recovery read on an AACS disc: the decrypting + // reader would reject it (DecryptFailed) and the sector would + // be abandoned without the drive ever being asked. Read the + // enclosing whole-unit window instead (units anchor at offset + // 0, so the start MUST be unit-aligned), decrypt that, then + // copy out the originally-requested [lba, count) window. All + // recovery accounting (pos, block_bytes, the dispatched + // lba/count) stays exactly as computed — only the physical + // read is widened, so the cursor cannot desync. + const U: u32 = 3; + let aligned_lba = lba - (lba % U); + let head = (lba - aligned_lba) as usize; // lead-in sectors + let span = head + count as usize; + let aligned_count = span + ((U as usize - span % U as usize) % U as usize); + let mut scratch = vec![0u8; aligned_count * 2048]; + match reader.read_sectors( + aligned_lba, + aligned_count as u16, + &mut scratch, + state.recovery, + ) { + Ok(_) => { + buf[..bytes] + .copy_from_slice(&scratch[head * 2048..head * 2048 + bytes]); + Ok(bytes) + } + Err(e) => Err(e), + } + } else { + reader.read_sectors(lba, count, &mut buf[..bytes], state.recovery) + }; let read_duration_ms = read_start.elapsed().as_millis(); match read_result { @@ -2758,4 +2814,55 @@ mod tests { ); } } + + /// Transport failure (status=0xFF, USB-bridge crash) must be recognised by + /// the gate `handle_read_failure` now checks FIRST, so it aborts the pass + /// (wedged_exit + BreakOuter) instead of treating the bridge crash as an + /// ordinary bad sector and hammering the crashed device for up to the + /// per-range watchdog budget. `handle_read_failure` is not unit-testable in + /// isolation, so this guards the classification predicate the production + /// early-return keys off, and the contrast that an ordinary read error is + /// NOT misclassified as a transport failure. + #[test] + fn transport_failure_is_recognised_for_patch_abort() { + use crate::scsi::SCSI_STATUS_TRANSPORT_FAILURE; + + // The exact shape Drive::read surfaces on a bridge crash. + let tf = Error::DiscRead { + sector: 1_392_314, + status: Some(SCSI_STATUS_TRANSPORT_FAILURE), + sense: None, + }; + assert!( + tf.is_scsi_transport_failure(), + "a DiscRead with status=0xFF must classify as a transport failure so \ + patch aborts the pass" + ); + + // The raw ScsiError form (e.g. straight from the transport) too. + let tf_raw = Error::ScsiError { + opcode: 0x28, + status: SCSI_STATUS_TRANSPORT_FAILURE, + sense: None, + }; + assert!(tf_raw.is_scsi_transport_failure()); + + // An ordinary recoverable bad sector (CHECK CONDITION with sense) must + // NOT trip the transport-failure abort — it should still be retried / + // marked NonTrimmed, not abort the whole pass. + let bad_sector = Error::DiscRead { + sector: 1_392_314, + status: Some(crate::scsi::SCSI_STATUS_CHECK_CONDITION), + sense: Some(crate::scsi::ScsiSense { + sense_key: 0x03, + asc: 0x11, + ascq: 0x00, + }), + }; + assert!( + !bad_sector.is_scsi_transport_failure(), + "an ordinary bad-sector CHECK CONDITION must not be misclassified as \ + a transport failure" + ); + } } diff --git a/src/scsi/windows.rs b/src/scsi/windows.rs index 9eef5e3..e29deb1 100644 --- a/src/scsi/windows.rs +++ b/src/scsi/windows.rs @@ -214,7 +214,13 @@ impl SptiTransport { /// Opens the device, sends IOCTL_STORAGE_RESET_DEVICE to reset /// the USB/SCSI bus, then closes. Same concept as SG_SCSI_RESET on Linux. pub fn reset(device: &Path) -> Result<()> { - const IOCTL_STORAGE_RESET_DEVICE: u32 = 0x002D1004; + // CTL_CODE(IOCTL_STORAGE_BASE=0x2D, 0x0400, METHOD_BUFFERED=0, + // FILE_READ_ACCESS|FILE_WRITE_ACCESS=3) + // = (0x2D<<16) | (3<<14) | (0x0400<<2) | 0 = 0x002DD000. + // The earlier literal 0x002D1004 decoded to function 0x401 with the + // access bits cleared — not IOCTL_STORAGE_RESET_DEVICE, so + // DeviceIoControl would fail ERROR_INVALID_FUNCTION instead of resetting. + const IOCTL_STORAGE_RESET_DEVICE: u32 = 0x002D_D000; let dev_str = device.to_str().ok_or_else(|| Error::DeviceNotFound { path: device.display().to_string(),