From b12476727c7a652e73c07a4af48c49a4c5d97e55 Mon Sep 17 00:00:00 2001 From: Guybrush <48859004+mcamposv@users.noreply.github.com> Date: Sun, 1 Nov 2026 16:03:25 +0100 Subject: [PATCH 1/2] SE050Sim: T=1 retransmission on R-blocks with error; store writes off the command path - An R-block with an error indication (the host did not get the last block, e.g. it gave up waiting) now gets the last single-frame response again. Before it got no answer, so the host, and any bridge relaying frames, waited forever. - With persistence enabled, the store was rewritten synchronously on every change, several times per command; on a busy disk that took up to a second, longer than NXP's T=1 host waits before asking for a retransmission. A background thread now writes the latest snapshot (temporary file, then rename). Unit tests keep the synchronous write. Unit test for the retransmission. Co-Authored-By: Claude Opus 5.5 --- SE050Sim/se050-sim/src/object_store/mod.rs | 38 +++++++++++++++++++++- SE050Sim/se050-sim/src/t1.rs | 29 +++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/SE050Sim/se050-sim/src/object_store/mod.rs b/SE050Sim/se050-sim/src/object_store/mod.rs index 4c53ac8..2d64557 100644 --- a/SE050Sim/se050-sim/src/object_store/mod.rs +++ b/SE050Sim/se050-sim/src/object_store/mod.rs @@ -91,6 +91,42 @@ fn default_curves() -> HashMap { (0x01..=0x05).map(|id| (id, CURVE_PARAMS_COMPLETE)).collect() } +/// Writes the persisted store without holding up the APDU being processed: +/// a background thread writes the latest snapshot (to a temporary file, then +/// renamed). A synchronous write in the command path took up to a second on a +/// busy disk, longer than hosts wait for an answer (NXP's T=1 stack then asks +/// for a retransmission). Unit tests write synchronously. +#[cfg(not(test))] +fn write_store(path: &std::path::Path, json: String) { + use std::sync::mpsc::{channel, Sender}; + use std::sync::{Mutex, OnceLock}; + static WRITER: OnceLock>> = OnceLock::new(); + let tx = WRITER.get_or_init(|| { + let (tx, rx) = channel::<(PathBuf, String)>(); + std::thread::spawn(move || { + while let Ok((mut p, mut j)) = rx.recv() { + while let Ok((p2, j2)) = rx.try_recv() { + p = p2; + j = j2; + } + let tmp = p.with_extension("json.tmp"); + if std::fs::write(&tmp, &j).is_ok() { + let _ = std::fs::rename(&tmp, &p); + } + } + }); + Mutex::new(tx) + }); + if let Ok(tx) = tx.lock() { + let _ = tx.send((path.to_path_buf(), json)); + } +} + +#[cfg(test)] +fn write_store(path: &std::path::Path, json: String) { + let _ = std::fs::write(path, json); +} + impl ObjectStore { pub fn new() -> Self { Self { @@ -287,7 +323,7 @@ impl ObjectStore { "platform_scp": self.platform_scp, }); if let Ok(json) = serde_json::to_string_pretty(&doc) { - let _ = std::fs::write(path, json); + write_store(path, json); } } diff --git a/SE050Sim/se050-sim/src/t1.rs b/SE050Sim/se050-sim/src/t1.rs index 64fb2ff..c77d3b8 100644 --- a/SE050Sim/se050-sim/src/t1.rs +++ b/SE050Sim/se050-sim/src/t1.rs @@ -166,6 +166,8 @@ pub struct T1Responder { /// PUT KEY is valid only after selecting the Security Domain, not while /// the IoT applet is selected. selected_ssd: bool, + /// Last single-frame response, for an R-block asking for a retransmission. + last_response: Vec>, } impl T1Responder { @@ -179,6 +181,7 @@ impl T1Responder { apdu_reassembly: Vec::new(), scp03: Scp03State::new(), selected_ssd: false, + last_response: Vec::new(), } } @@ -204,6 +207,13 @@ impl T1Responder { FrameType::IFrame { seq, multi } => { self.handle_i_frame(seq, multi, &frame.payload, store) } + FrameType::RFrame { err, .. } if err != 0 => { + // R-block with an error indication: the host did not get our + // last block (e.g. it gave up waiting) and asks for it again. + // Without an answer the host and a bridge in between would + // wait forever. + self.last_response.clone() + } FrameType::RFrame { .. } => { // R-frames during multi-frame response sending // Currently we pre-queue all response frames, so we just ignore R-frames @@ -413,7 +423,9 @@ impl T1Responder { let (header, payload_crc) = build_frame(self.nad_se2hd, ft, response_bytes); chunks.push(header); chunks.push(payload_crc); + self.last_response = chunks.clone(); } else { + self.last_response.clear(); // Multi-frame response let mut offset = 0; while offset < response_bytes.len() { @@ -444,6 +456,23 @@ impl T1Responder { mod tests { use super::*; + #[test] + fn test_r_block_with_error_repeats_last_response() { + let mut t1 = T1Responder::new(0x5A); + let mut store = ObjectStore::new(); + // I-block 0 carrying SELECT of the IoT applet + let select = [0x00, 0xA4, 0x04, 0x00, 0x10, 0xA0, 0x00, 0x00, 0x03, 0x96, 0x54, 0x53, 0x00, 0x00, 0x00, 0x01, 0x03, 0x00, 0x00, 0x00, 0x00]; + let (h, p) = build_frame(0x5A, FrameType::IFrame { seq: 0, multi: false }, &select); + let first = t1.process_frame(&[h, p].concat(), &mut store); + assert_eq!(first.len(), 2); + // R-block, N(R) = 1, error bits set: the same response again + let (h, p) = build_frame(0x5A, FrameType::RFrame { seq: 1, err: 0x01 }, &[]); + assert_eq!(t1.process_frame(&[h, p].concat(), &mut store), first); + // without error bits (chaining acknowledgement) nothing is sent + let (h, p) = build_frame(0x5A, FrameType::RFrame { seq: 1, err: 0x00 }, &[]); + assert!(t1.process_frame(&[h, p].concat(), &mut store).is_empty()); + } + #[test] fn test_crc16() { // From driver test: assert_eq!(0x78a1, Se050CRC::calculate(&[0,48,95,111,242])); From 4e076b05f9d6e09162e8876653a249cb263279f2 Mon Sep 17 00:00:00 2001 From: Guybrush <48859004+mcamposv@users.noreply.github.com> Date: Mon, 2 Nov 2026 07:06:29 +0100 Subject: [PATCH 2/2] SE050Sim: address review - store writer lifecycle and permissions, T=1 reset Fixes from review feedback on the T=1 retransmission and background store writes. The background writer kept only the last queued snapshot, whatever its destination, so two persistent stores in one process could lose each other's updates. Snapshots are now coalesced per path. Writes returned before reaching the disk and nothing waited for them, so a store dropped and reopened on the same file could load stale state: tests/scp03.rs set_platform_scp_request_flow failed on the reload of scp_required. There is now a flush barrier per path: loading a store file waits for its pending write, and dropping a persistent store waits for its own. APDU handling still never waits for the disk. The replacement file was created with the umask, so a store chmodded to 0600 became world-readable on its next update, and the temporary file was readable while being written. The store holds private keys and SCP03 keys: the temporary file is now created with mode 0600 and takes the mode of the file it replaces, if any. The cached response for an error R-block survived InterfaceSoftReset and resync, so a host could get a response from the previous link replayed. Both resets now clear it. Tests: per-path coalescing with two stores, reopening a store while the first is still open, file modes (new file 0600, existing mode kept), and an error R-block after each reset. Co-Authored-By: Claude Opus 5.5 --- SE050Sim/se050-sim/src/object_store/mod.rs | 195 ++++++++++++++++++--- SE050Sim/se050-sim/src/t1.rs | 20 +++ 2 files changed, 188 insertions(+), 27 deletions(-) diff --git a/SE050Sim/se050-sim/src/object_store/mod.rs b/SE050Sim/se050-sim/src/object_store/mod.rs index 2d64557..5b8752d 100644 --- a/SE050Sim/se050-sim/src/object_store/mod.rs +++ b/SE050Sim/se050-sim/src/object_store/mod.rs @@ -92,39 +92,125 @@ fn default_curves() -> HashMap { } /// Writes the persisted store without holding up the APDU being processed: -/// a background thread writes the latest snapshot (to a temporary file, then -/// renamed). A synchronous write in the command path took up to a second on a -/// busy disk, longer than hosts wait for an answer (NXP's T=1 stack then asks -/// for a retransmission). Unit tests write synchronously. -#[cfg(not(test))] -fn write_store(path: &std::path::Path, json: String) { - use std::sync::mpsc::{channel, Sender}; - use std::sync::{Mutex, OnceLock}; - static WRITER: OnceLock>> = OnceLock::new(); - let tx = WRITER.get_or_init(|| { - let (tx, rx) = channel::<(PathBuf, String)>(); - std::thread::spawn(move || { - while let Ok((mut p, mut j)) = rx.recv() { - while let Ok((p2, j2)) = rx.try_recv() { - p = p2; - j = j2; +/// a background thread writes the latest snapshot of each store file. A +/// synchronous write in the command path took up to a second on a busy disk, +/// longer than hosts wait for an answer (NXP's T=1 stack then asks for a +/// retransmission). +/// +/// - Snapshots are coalesced per destination path, so several stores in one +/// process never discard each other's updates. +/// - `flush_store` waits until a path has no queued or in-flight write; it runs +/// before a store file is loaded and when a persistent store is dropped, so +/// reopening a store never reads stale state. +/// - The file holds private keys and SCP03 keys: it is written to a temporary +/// file created with mode 0600 (or the existing file's mode), then renamed. +mod writer { + use std::collections::{HashMap, HashSet}; + use std::path::{Path, PathBuf}; + use std::sync::{Condvar, Mutex, OnceLock}; + + #[derive(Default)] + struct State { + pending: HashMap, + in_flight: HashSet, + } + + struct Writer { + state: Mutex, + cv: Condvar, + } + + static WRITER: OnceLock<&'static Writer> = OnceLock::new(); + + fn writer() -> &'static Writer { + WRITER.get_or_init(|| { + let w: &'static Writer = Box::leak(Box::new(Writer { + state: Mutex::new(State::default()), + cv: Condvar::new(), + })); + std::thread::spawn(move || run(w)); + w + }) + } + + fn run(w: &'static Writer) { + loop { + let batch: Vec<(PathBuf, String)> = { + let mut st = w.state.lock().unwrap_or_else(|e| e.into_inner()); + while st.pending.is_empty() { + st = w.cv.wait(st).unwrap_or_else(|e| e.into_inner()); } - let tmp = p.with_extension("json.tmp"); - if std::fs::write(&tmp, &j).is_ok() { - let _ = std::fs::rename(&tmp, &p); + let batch: Vec<_> = st.pending.drain().collect(); + for (p, _) in &batch { + st.in_flight.insert(p.clone()); } + batch + }; + for (path, json) in &batch { + write_file(path, json); } - }); - Mutex::new(tx) - }); - if let Ok(tx) = tx.lock() { - let _ = tx.send((path.to_path_buf(), json)); + let mut st = w.state.lock().unwrap_or_else(|e| e.into_inner()); + for (p, _) in &batch { + st.in_flight.remove(p); + } + w.cv.notify_all(); + } + } + + fn write_file(path: &Path, json: &str) { + use std::io::Write; + let mut tmp_name = path.as_os_str().to_owned(); + tmp_name.push(".tmp"); + let tmp = PathBuf::from(tmp_name); + let _ = std::fs::remove_file(&tmp); + let mut opts = std::fs::OpenOptions::new(); + opts.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + opts.mode(0o600); + } + let Ok(mut f) = opts.open(&tmp) else { return }; + // Keep the access restrictions of the file being replaced. + if let Ok(meta) = std::fs::metadata(path) { + let _ = f.set_permissions(meta.permissions()); + } + if f.write_all(json.as_bytes()).is_ok() && f.flush().is_ok() { + drop(f); + let _ = std::fs::rename(&tmp, path); + } else { + drop(f); + let _ = std::fs::remove_file(&tmp); + } + } + + pub fn write_store(path: &Path, json: String) { + let w = writer(); + let mut st = w.state.lock().unwrap_or_else(|e| e.into_inner()); + st.pending.insert(path.to_path_buf(), json); + w.cv.notify_all(); + } + + /// Blocks until `path` has no queued or in-flight write. + pub fn flush_store(path: &Path) { + let Some(w) = WRITER.get() else { return }; + let mut st = w.state.lock().unwrap_or_else(|e| e.into_inner()); + while st.pending.contains_key(path) || st.in_flight.contains(path) { + st = w.cv.wait(st).unwrap_or_else(|e| e.into_inner()); + } } } -#[cfg(test)] -fn write_store(path: &std::path::Path, json: String) { - let _ = std::fs::write(path, json); +use writer::{flush_store, write_store}; + +/// A persistent store finishes writing its file before it goes away, so a +/// store reopened on the same file (or an orderly shutdown) sees every update. +impl Drop for ObjectStore { + fn drop(&mut self) { + if let Some(path) = &self.persist_path { + flush_store(path); + } + } } impl ObjectStore { @@ -329,6 +415,7 @@ impl ObjectStore { fn load(&mut self) { let Some(path) = &self.persist_path else { return }; + flush_store(path); let Ok(json) = std::fs::read_to_string(path) else { return }; let Ok(value): Result = serde_json::from_str(&json) else { return; @@ -503,4 +590,58 @@ mod persistence_tests { assert_eq!(loaded.dek, config.dek); let _ = std::fs::remove_file(&path); } + + #[test] + fn test_stores_on_different_paths_keep_their_latest_snapshot() { + let a = unique_store_path("writer_path_a"); + let b = unique_store_path("writer_path_b"); + { + let mut sa = ObjectStore::with_persistence(a.clone()); + let mut sb = ObjectStore::with_persistence(b.clone()); + for i in 0..50u8 { + sa.insert([0, 0, 0xA0, i], SecureObject::Binary { data: vec![i] }); + sb.insert([0, 0, 0xB0, i], SecureObject::Binary { data: vec![i] }); + } + } + let sa = ObjectStore::with_persistence(a.clone()); + let sb = ObjectStore::with_persistence(b.clone()); + for i in 0..50u8 { + assert!(sa.exists(&[0, 0, 0xA0, i]) && !sa.exists(&[0, 0, 0xB0, i])); + assert!(sb.exists(&[0, 0, 0xB0, i]) && !sb.exists(&[0, 0, 0xA0, i])); + } + let _ = std::fs::remove_file(&a); + let _ = std::fs::remove_file(&b); + } + + #[test] + fn test_reopen_sees_updates_of_a_store_still_open() { + let path = unique_store_path("writer_reopen"); + let mut first = ObjectStore::with_persistence(path.clone()); + first.set_scp_required(true); + let second = ObjectStore::with_persistence(path.clone()); + assert!(second.scp_required()); + drop(first); + let _ = std::fs::remove_file(&path); + } + + #[cfg(unix)] + #[test] + fn test_store_file_is_private_and_keeps_its_mode() { + use std::os::unix::fs::PermissionsExt; + let path = unique_store_path("writer_mode"); + let mode = |p: &PathBuf| std::fs::metadata(p).unwrap().permissions().mode() & 0o777; + { + let mut store = ObjectStore::with_persistence(path.clone()); + store.insert([0, 0, 0, 1], SecureObject::Binary { data: vec![1] }); + } + assert_eq!(mode(&path), 0o600); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o640)).unwrap(); + { + let mut store = ObjectStore::with_persistence(path.clone()); + store.insert([0, 0, 0, 2], SecureObject::Binary { data: vec![2] }); + } + assert_eq!(mode(&path), 0o640); + assert!(ObjectStore::with_persistence(path.clone()).exists(&[0, 0, 0, 2])); + let _ = std::fs::remove_file(&path); + } } diff --git a/SE050Sim/se050-sim/src/t1.rs b/SE050Sim/se050-sim/src/t1.rs index c77d3b8..abfca69 100644 --- a/SE050Sim/se050-sim/src/t1.rs +++ b/SE050Sim/se050-sim/src/t1.rs @@ -233,6 +233,8 @@ impl T1Responder { self.apdu_reassembly.clear(); self.scp03.reset(); self.selected_ssd = false; + // Nothing is pending on a fresh link. + self.last_response.clear(); let ft = FrameType::SFrame { code: T1_S_INTERFACE_SOFT_RESET, @@ -248,6 +250,8 @@ impl T1Responder { self.apdu_reassembly.clear(); self.scp03.reset(); self.selected_ssd = false; + // Nothing is pending on a fresh link. + self.last_response.clear(); let ft = FrameType::SFrame { code: 0x00, is_response: true }; let (header, payload_crc) = build_frame(self.nad_se2hd, ft, &[]); @@ -473,6 +477,22 @@ mod tests { assert!(t1.process_frame(&[h, p].concat(), &mut store).is_empty()); } + #[test] + fn test_reset_and_resync_forget_last_response() { + let select = [0x00, 0xA4, 0x04, 0x00, 0x10, 0xA0, 0x00, 0x00, 0x03, 0x96, 0x54, 0x53, 0x00, 0x00, 0x00, 0x01, 0x03, 0x00, 0x00, 0x00, 0x00]; + for code in [T1_S_INTERFACE_SOFT_RESET, 0x00] { + let mut t1 = T1Responder::new(0x5A); + let mut store = ObjectStore::new(); + let (h, p) = build_frame(0x5A, FrameType::IFrame { seq: 0, multi: false }, &select); + assert_eq!(t1.process_frame(&[h, p].concat(), &mut store).len(), 2); + let (h, p) = build_frame(0x5A, FrameType::SFrame { code, is_response: false }, &[]); + assert_eq!(t1.process_frame(&[h, p].concat(), &mut store).len(), 2); + // An error R-block after the reset has no previous response to replay. + let (h, p) = build_frame(0x5A, FrameType::RFrame { seq: 0, err: 0x01 }, &[]); + assert!(t1.process_frame(&[h, p].concat(), &mut store).is_empty(), "S-block {code:#04x}"); + } + } + #[test] fn test_crc16() { // From driver test: assert_eq!(0x78a1, Se050CRC::calculate(&[0,48,95,111,242]));