firmware: SET/RESET need a data-in phase (drive aborts no-data verbs); IDENTITY is a version banner, not a binary state table

This commit is contained in:
Matthew Jackson
2026-09-15 18:02:22 -07:00
parent f1801351e2
commit 4459af4e2f
3 changed files with 58 additions and 90 deletions
+1 -1
View File
@@ -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"
);
+24 -47
View File
@@ -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 <version>` (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<Self> {
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.
+33 -42
View File
@@ -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 <ver>`,
// 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 <ver>` 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<Vec<u8>>,
@@ -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<ScsiResult> {
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 <ver>` 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));