From 4459af4e2fecdf7d054f609ef119ecef460084f4 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Tue, 15 Sep 2026 18:02:22 -0700 Subject: [PATCH] firmware: SET/RESET need a data-in phase (drive aborts no-data verbs); IDENTITY is a version banner, not a binary state table --- src/aacs/mod.rs | 2 +- src/firmware/mod.rs | 71 +++++++++++++----------------------- src/firmware/mod_tests.rs | 75 +++++++++++++++++---------------------- 3 files changed, 58 insertions(+), 90 deletions(-) diff --git a/src/aacs/mod.rs b/src/aacs/mod.rs index 868721d..2e23e05 100644 --- a/src/aacs/mod.rs +++ b/src/aacs/mod.rs @@ -119,7 +119,7 @@ impl AacsUnlocker { tracing::debug!( target: "freemkv::disc", phase = "aacs_arm_freemkv_detected", - version = id.version, + version = %id.version, recipe = ?recipe, "freemkv firmware detected; arming before the cert AKE" ); diff --git a/src/firmware/mod.rs b/src/firmware/mod.rs index 9d1ceee..c4a9429 100644 --- a/src/firmware/mod.rs +++ b/src/firmware/mod.rs @@ -342,45 +342,32 @@ impl FeatureStates { Feature::Bus => self.bus = state, } } - - /// Parse a feature-state table (the bytes AFTER magic+version in an IDENTITY - /// reply) in [`ALL_FEATURES`] order. Missing trailing bytes default to - /// [`STATE_PASSTHROUGH`] so a short/older reply parses defensively. - pub fn from_table(table: &[u8]) -> Self { - let mut s = FeatureStates::all_passthrough(); - for (i, feature) in ALL_FEATURES.iter().enumerate() { - if let Some(&b) = table.get(i) { - s.set(*feature, b); - } - } - s - } } -/// A parsed [`Verb::Identity`] reply: the firmware version plus the current -/// feature-state table. Only present when the reply led with [`RESP_MAGIC`]. +/// A parsed [`Verb::Identity`] reply. Only present when the reply led with +/// [`RESP_MAGIC`]. The firmware answers IDENTITY with a human banner +/// `freemkv ` (NUL-padded) — there is NO binary state table in the +/// reply, so this carries only the version string; read the live feature states +/// via [`FirmwareControl::states`]/[`get`](FirmwareControl::get). #[derive(Debug, Clone, PartialEq, Eq)] pub struct FirmwareIdentity { - /// Firmware version byte (the byte after [`RESP_MAGIC`]). - pub version: u8, - /// The feature-state table reported by the drive. - pub states: FeatureStates, + /// Firmware version string, the banner text after [`RESP_MAGIC`] up to the + /// first NUL, trimmed (e.g. `"0.7.1"`). + pub version: String, } impl FirmwareIdentity { /// Parse an IDENTITY data-in payload. `None` unless it leads with /// [`RESP_MAGIC`] (i.e. not freemkv firmware). Layout: `RESP_MAGIC` (7) + - /// version (1) + feature-state table (one byte per [`ALL_FEATURES`]). + /// ASCII version banner, NUL-terminated/padded. pub fn parse(bytes: &[u8]) -> Option { if !verify_response(bytes) { return None; } - let version = bytes.get(RESP_MAGIC.len()).copied().unwrap_or(0); - let table = bytes.get(RESP_MAGIC.len() + 1..).unwrap_or(&[]); - Some(FirmwareIdentity { - version, - states: FeatureStates::from_table(table), - }) + let tail = &bytes[RESP_MAGIC.len()..]; + let end = tail.iter().position(|&b| b == 0).unwrap_or(tail.len()); + let version = String::from_utf8_lossy(&tail[..end]).trim().to_string(); + Some(FirmwareIdentity { version }) } } @@ -447,24 +434,7 @@ impl<'a> FirmwareControl<'a> { } } - /// Issue a no-data CDB (SET / RESET), caring only about GOOD status. - fn exec_none(&mut self, cdb: &[u8; CDB_LEN]) -> Result<()> { - let mut empty: [u8; 0] = []; - match self - .scsi - .execute(cdb, DataDirection::None, &mut empty, CMD_TIMEOUT_MS) - { - Ok(r) if r.status == 0 => Ok(()), - Ok(_) => Err(FirmwareError::Rejected), - Err(e) => Err(if is_dead_bus(&e) { - FirmwareError::Transport - } else { - FirmwareError::Rejected - }), - } - } - - /// Send IDENTITY and parse the magic + version + feature-state table. + /// Send IDENTITY and parse the magic + version banner. /// `Ok(None)` if the drive answered but the reply lacked [`RESP_MAGIC`] (not /// freemkv firmware); `Err(Transport)` only on a dead bus, `Err(Rejected)` /// if the drive refused the command outright (also "not freemkv"). @@ -495,15 +465,22 @@ impl<'a> FirmwareControl<'a> { } /// Set one feature to an explicit state byte (SET). + /// + /// Issued with a data-in phase (`exec_in`), not `exec_none`: the drive's + /// READ BUFFER hijack aborts ANY vendor verb that carries no data-in + /// transfer — the `MIN_ALLOC_LEN` floor in the CDB is only honored when the + /// host actually reads that many bytes back. The returned bytes are the + /// echoed state frame; we discard them (GET is the read path). pub fn set(&mut self, feature: Feature, state: u8) -> Result<()> { let cdb = build_set_cdb(feature, state); - self.exec_none(&cdb) + self.exec_in(&cdb).map(|_| ()) } - /// Restore every feature to [`STATE_PASSTHROUGH`] (RESET). + /// Restore every feature to [`STATE_PASSTHROUGH`] (RESET). Data-in phase for + /// the same reason as [`set`](Self::set) — a no-data RESET is aborted. pub fn reset(&mut self) -> Result<()> { let cdb = build_reset_cdb(); - self.exec_none(&cdb) + self.exec_in(&cdb).map(|_| ()) } /// SET a feature, then GET it back and confirm the state stuck. diff --git a/src/firmware/mod_tests.rs b/src/firmware/mod_tests.rs index 43511a5..a2a48c1 100644 --- a/src/firmware/mod_tests.rs +++ b/src/firmware/mod_tests.rs @@ -117,29 +117,15 @@ fn region_state_bytes() { // ── Identity / states parsing ──────────────────────────────────────────────── #[test] -fn identity_parse_reads_version_and_table() { +fn identity_parse_reads_version_banner() { + // The firmware answers IDENTITY with the ASCII banner `freemkv `, + // NUL-padded — there is no binary state table. let mut payload = Vec::new(); payload.extend_from_slice(RESP_MAGIC); - payload.push(0x17); // version - // table in ALL_FEATURES order: Speed, Region, Uhd, Bd, Hrl, Ake, Bus - payload.extend_from_slice(&[ - SPEED_MAX, - STATE_ON, - STATE_ON, - STATE_PASSTHROUGH, - STATE_ON, - STATE_OFF, - STATE_ON, - ]); + payload.extend_from_slice(b" 0.7.1\0"); + payload.resize(64, 0); // NUL-pad to the allocation window let id = FirmwareIdentity::parse(&payload).expect("has magic"); - assert_eq!(id.version, 0x17); - assert_eq!(id.states.speed, SPEED_MAX); - assert_eq!(id.states.region, STATE_ON); - assert_eq!(id.states.uhd, STATE_ON); - assert_eq!(id.states.bd, STATE_PASSTHROUGH); - assert_eq!(id.states.hrl, STATE_ON); - assert_eq!(id.states.ake, STATE_OFF); - assert_eq!(id.states.bus, STATE_ON); + assert_eq!(id.version, "0.7.1"); } #[test] @@ -147,23 +133,17 @@ fn identity_parse_none_without_magic() { assert!(FirmwareIdentity::parse(&[0u8; 64]).is_none()); } -#[test] -fn identity_parse_short_table_defaults_to_passthrough() { - let mut payload = Vec::new(); - payload.extend_from_slice(RESP_MAGIC); - payload.push(0x01); - // no table bytes at all - let id = FirmwareIdentity::parse(&payload).expect("has magic"); - assert_eq!(id.states, FeatureStates::all_passthrough()); -} - // ── A stateful firmware mock ───────────────────────────────────────────────── -/// A mock freemkv drive: answers IDENTITY with magic+version+table, GET with the -/// requested feature's state, applies SET, and RESET → all-passthrough. Records -/// every CDB so tests can assert the exact command order. +/// A mock freemkv drive: answers IDENTITY with the `freemkv ` banner, GET +/// with the requested feature's state, applies SET, and RESET → all-passthrough. +/// Records every CDB so tests can assert the exact command order. +/// +/// It also models the real hardware constraint: any vendor verb executed with +/// [`DataDirection::None`] (no data-in phase) is REJECTED with a non-zero +/// status, exactly as the drive's READ BUFFER hijack aborts a no-data verb. struct FwMock { - version: u8, + version: &'static str, states: FeatureStates, is_freemkv: bool, cdbs: Vec>, @@ -172,7 +152,7 @@ struct FwMock { impl FwMock { fn new() -> Self { FwMock { - version: 0x42, + version: "0.6.6", states: FeatureStates::all_passthrough(), is_freemkv: true, cdbs: Vec::new(), @@ -194,21 +174,31 @@ impl ScsiTransport for FwMock { fn execute( &mut self, cdb: &[u8], - _dir: DataDirection, + dir: DataDirection, data: &mut [u8], _timeout_ms: u32, ) -> ScsiTResult { self.cdbs.push(cdb.to_vec()); + // Model the hardware: a vendor verb with no data-in phase is aborted. + // This is what makes the old `exec_none` SET/RESET path a test failure. + let is_vendor = cdb[CDB_OPCODE] == READ_BUFFER_OPCODE && cdb[CDB_MODE] == KNOCK_MODE; + if is_vendor && matches!(dir, DataDirection::None) { + return Ok(ScsiResult { + status: 0x02, // CHECK CONDITION — rejected + bytes_transferred: 0, + sense: [0u8; 32], + }); + } let mut transferred = 0usize; match cdb[CDB_VERB] { v if v == Verb::Identity as u8 => { if self.is_freemkv { + // The `freemkv ` banner, NUL-terminated. let mut resp = Vec::new(); resp.extend_from_slice(RESP_MAGIC); - resp.push(self.version); - for f in ALL_FEATURES { - resp.push(self.states.get(f)); - } + resp.push(b' '); + resp.extend_from_slice(self.version.as_bytes()); + resp.push(0); let n = resp.len().min(data.len()); data[..n].copy_from_slice(&resp[..n]); transferred = n; @@ -257,8 +247,9 @@ fn identity_detects_freemkv_and_issues_identity_cdb() { { let mut fw = FirmwareControl::new(&mut m); let id = fw.identity().expect("no fault").expect("is freemkv"); - assert_eq!(id.version, 0x42); - assert_eq!(id.states.ake, STATE_ON); + assert_eq!(id.version, "0.6.6"); + // States no longer ride in IDENTITY — read them via GET. + assert_eq!(fw.get(Feature::Ake).expect("get"), STATE_ON); assert!(fw.is_freemkv().expect("no fault")); } assert_eq!(m.cdbs[0], build_identity_cdb(MEMREAD_LEN as u16));