From ddf069c0384adad4763ec71eb661d74186d1f271 Mon Sep 17 00:00:00 2001 From: Stephane Portron Date: Fri, 21 Aug 2026 10:33:13 +0200 Subject: [PATCH 1/4] Fix: If incoming APDU occurs during previous APDU processing then send a specific error code. --- ledger_device_sdk/src/io_legacy.rs | 8 +++++++- ledger_device_sdk/src/io_new/callbacks.rs | 5 +++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/ledger_device_sdk/src/io_legacy.rs b/ledger_device_sdk/src/io_legacy.rs index 9476c7c9..e2f5e5ab 100644 --- a/ledger_device_sdk/src/io_legacy.rs +++ b/ledger_device_sdk/src/io_legacy.rs @@ -44,6 +44,7 @@ pub enum StatusWords { Unknown = 0x6d00, Panic = 0xe000, DeviceLocked = 0x5515, + CmdNotAccepted = 0x6901, } #[derive(Debug)] @@ -363,7 +364,12 @@ impl Comm { let status = sys_seph::io_rx(&mut self.io_buffer, true); if status > 0 { - return self.detect_apdu::(status); + let is_apdu = self.detect_apdu::(status); + if is_apdu && self.event_pending { + self.reply(Reply(StatusWords::CmdNotAccepted as u16)); + return false; + } + return is_apdu; } false } diff --git a/ledger_device_sdk/src/io_new/callbacks.rs b/ledger_device_sdk/src/io_new/callbacks.rs index b77a5eec..2c983b1a 100644 --- a/ledger_device_sdk/src/io_new/callbacks.rs +++ b/ledger_device_sdk/src/io_new/callbacks.rs @@ -3,7 +3,7 @@ //! This module holds the erased pointer to the current `Comm` instance and the //! generic callback wrappers that are registered through `nbgl_register_callbacks`. -use crate::io_legacy::{ApduHeader, Reply}; +use crate::io_legacy::{ApduHeader, Reply, StatusWords}; #[cfg(feature = "stack_usage")] use super::bolos::handle_bolos_apdu; @@ -73,7 +73,8 @@ pub(super) fn next_event_ahead_impl() -> bool { // fetching another event. This prevents consuming the same APDU repeatedly // when ux_sync_wait loops with exit_on_apdu=false. if comm.pending_apdu { - return true; + reply_status_impl::(Reply(StatusWords::CmdNotAccepted as u16)); + return false; } match comm.next_event().into_type() { DecodedEventType::Apdu { From fd08ca0fdea0c871ac2a5f5ab42073b155d56ecb Mon Sep 17 00:00:00 2001 From: Stephane Portron Date: Mon, 24 Aug 2026 09:41:43 +0200 Subject: [PATCH 2/4] PR review comment resolution --- ledger_device_sdk/src/io_legacy.rs | 4 ++-- ledger_device_sdk/src/io_new/callbacks.rs | 4 +--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/ledger_device_sdk/src/io_legacy.rs b/ledger_device_sdk/src/io_legacy.rs index e2f5e5ab..f60c2257 100644 --- a/ledger_device_sdk/src/io_legacy.rs +++ b/ledger_device_sdk/src/io_legacy.rs @@ -301,6 +301,7 @@ impl Comm { } self.tx_length = 0; self.rx_length = 0; + self.event_pending = false; } /// Wait and return next button press event or APDU command. @@ -365,11 +366,10 @@ impl Comm { if status > 0 { let is_apdu = self.detect_apdu::(status); - if is_apdu && self.event_pending { + if is_apdu { self.reply(Reply(StatusWords::CmdNotAccepted as u16)); return false; } - return is_apdu; } false } diff --git a/ledger_device_sdk/src/io_new/callbacks.rs b/ledger_device_sdk/src/io_new/callbacks.rs index 2c983b1a..78249f2c 100644 --- a/ledger_device_sdk/src/io_new/callbacks.rs +++ b/ledger_device_sdk/src/io_new/callbacks.rs @@ -69,9 +69,7 @@ fn panic_reply_impl(reply: Reply) { pub(super) fn next_event_ahead_impl() -> bool { let comm = unsafe { get_comm::() }; - // If there's already a pending APDU, return true immediately without - // fetching another event. This prevents consuming the same APDU repeatedly - // when ux_sync_wait loops with exit_on_apdu=false. + // If there's already a pending APDU, return Cmd not accepted error status if comm.pending_apdu { reply_status_impl::(Reply(StatusWords::CmdNotAccepted as u16)); return false; From 06971c9bd1b2131cc594bf87ecbdfff9f6bcff3f Mon Sep 17 00:00:00 2001 From: GroM Date: Mon, 24 Aug 2026 16:27:08 +0200 Subject: [PATCH 3/4] io_legacy: gate double-APDU rejection on an explicit in-progress flag `next_event_ahead` is called from `ux_sync_wait` in two different situations, and rejecting unconditionally broke the first one: * no command being processed (idle home screen): an incoming APDU is the normal way of receiving work. Returning `false` here meant `ux_sync_wait(true)` could never report `UxSyncRetApduReceived`, so an app using the blocking `NbglHomeAndSettings::show()` answered 0x6901 to every APDU and became unreachable from the host. * a command being processed (review, status, ... screen): an incoming APDU is a double APDU and must be answered CmdNotAccepted. io_legacy had no state distinguishing the two: `event_pending` cannot serve, as `check_event` clears it before handing the command to the application. Add `Comm::apdu_in_progress`, set when a command is handed to the application and cleared when the application replies, and gate the rejection on it. This also lets `apdu_send` stop clearing `event_pending`, which silently discarded queued commands on every reply path instead of answering them. Take the decision on the raw frame rather than after `detect_apdu`: `decode_event` overwrites `apdu_buffer`, `apdu_type`, `rx` and `rx_length`, i.e. state owned by the command still being processed. BOLOS APDUs (CLA 0xB0) and frames too short to hold a header keep falling through to `check_event` so their existing handling is preserved. Reject via a dedicated `reject_apdu` that transmits the status word on the intruder's own transport and leaves every response and receive field untouched. Going through `Comm::reply`/`apdu_send` instead would flush any bytes the application had staged before showing the screen (sending them to the host with the error, then losing them from the real response), overwrite `apdu_type` so the in-flight command replied on the wrong channel, and consume a SEPH event, which can drop the user's tap. Also mark `NbglHomeAndSettings::show()` with `#[deprecated]`, which its documentation already stated in prose only, and bump to 1.37.0: the new `StatusWords::CmdNotAccepted` variant breaks downstream exhaustive matches over that public enum. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.lock | 2 +- ledger_device_sdk/CHANGELOG.md | 13 ++++ ledger_device_sdk/Cargo.toml | 2 +- ledger_device_sdk/src/io_legacy.rs | 70 ++++++++++++++++--- .../src/nbgl/nbgl_home_and_settings.rs | 12 ++-- 5 files changed, 85 insertions(+), 14 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f8ec1205..3492f940 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -196,7 +196,7 @@ checksum = "92ecc6618181def0457392ccd0ee51198e065e016d1d527a7ac1b6dc7c1f09d2" [[package]] name = "ledger_device_sdk" -version = "1.36.2" +version = "1.37.0" dependencies = [ "const-zero", "include_gif", diff --git a/ledger_device_sdk/CHANGELOG.md b/ledger_device_sdk/CHANGELOG.md index b6ebe4f1..517cdc5c 100644 --- a/ledger_device_sdk/CHANGELOG.md +++ b/ledger_device_sdk/CHANGELOG.md @@ -5,6 +5,19 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [1.37.0] - 2026-08-24 + +### Added +- `StatusWords::CmdNotAccepted` (0x6901). An APDU received while a previous + command is still being processed is now answered with this status word instead + of being queued or silently dropped. Note that adding a variant to the public + `StatusWords` enum breaks downstream exhaustive `match` expressions over it. + +### Deprecated +- `NbglHomeAndSettings::show()` is now marked with the `#[deprecated]` attribute, + matching what its documentation already stated. Use `show_and_return()` + instead, which does not force a home screen refresh for every received APDU. + ## [1.36.2] - 2026-08-18 ### Changed diff --git a/ledger_device_sdk/Cargo.toml b/ledger_device_sdk/Cargo.toml index cb00018d..105ad2ab 100644 --- a/ledger_device_sdk/Cargo.toml +++ b/ledger_device_sdk/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "ledger_device_sdk" -version = "1.36.2" +version = "1.37.0" authors = ["Ledger"] edition = "2024" license.workspace = true diff --git a/ledger_device_sdk/src/io_legacy.rs b/ledger_device_sdk/src/io_legacy.rs index f60c2257..bfd40b4b 100644 --- a/ledger_device_sdk/src/io_legacy.rs +++ b/ledger_device_sdk/src/io_legacy.rs @@ -195,6 +195,10 @@ pub struct Comm { /// Used when replying to BOLOS APDUs where io_rx(false) would deadlock. #[allow(dead_code)] skip_rx_on_send: bool, + /// True from the moment a command is handed to the application until the + /// application replies to it. This is what makes a second incoming APDU a + /// *double* APDU rather than the normal way of receiving work. + apdu_in_progress: bool, } impl Default for Comm { @@ -232,6 +236,7 @@ impl Comm { rx_length: 0, tx_length: 0, skip_rx_on_send: false, + apdu_in_progress: false, } } @@ -301,7 +306,8 @@ impl Comm { } self.tx_length = 0; self.rx_length = 0; - self.event_pending = false; + // Replying completes the current command. + self.apdu_in_progress = false; } /// Wait and return next button press event or APDU command. @@ -357,21 +363,41 @@ impl Comm { } } + /// Fetch and process one event while an NBGL screen is displayed, and report + /// whether it was an APDU command. + /// + /// This is called from `ux_sync_wait` in two different situations: + /// + /// * no command is being processed (idle home screen): an incoming APDU is + /// the normal way of receiving work, so it is decoded and `true` is + /// returned so the caller can leave the screen and handle it. + /// * a command is being processed (review, status, … screen): an incoming + /// APDU is a double APDU and is answered [`StatusWords::CmdNotAccepted`]. pub fn next_event_ahead(&mut self) -> bool where T: TryFrom, Reply: From<>::Error>, { let status = sys_seph::io_rx(&mut self.io_buffer, true); + if status <= 0 { + return false; + } - if status > 0 { - let is_apdu = self.detect_apdu::(status); - if is_apdu { - self.reply(Reply(StatusWords::CmdNotAccepted as u16)); - return false; - } + // Reject a double APDU on the raw frame, before any decoding: the + // in-flight command owns apdu_buffer / apdu_type / rx / tx, so the + // intruder must never be decoded into them. BOLOS APDUs (CLA 0xB0) and + // frames too short to hold a header fall through to keep their existing + // handling in `check_event`. + if self.apdu_in_progress + && Self::is_apdu_packet(self.io_buffer[0]) + && status >= 5 + && self.io_buffer[1] != 0xB0 + { + self.reject_apdu(StatusWords::CmdNotAccepted); + return false; } - false + + self.detect_apdu::(status) } pub fn check_event(&mut self) -> Option> @@ -419,6 +445,9 @@ impl Comm { let res = T::try_from(*self.get_apdu_metadata()); match res { Ok(ins) => { + // The command is about to be handed to the application: any + // APDU arriving from now until the reply is a double APDU. + self.apdu_in_progress = true; return Some(Event::Command(ins)); } Err(sw) => { @@ -608,6 +637,31 @@ impl Comm { } } + /// True if a received SEPH frame carries an APDU rather than an event. + fn is_apdu_packet(packet_type: u8) -> bool { + matches!( + seph::PacketTypes::from(packet_type), + seph::PacketTypes::PacketTypeRawApdu + | seph::PacketTypes::PacketTypeUsbHidApdu + | seph::PacketTypes::PacketTypeUsbWebusbApdu + | seph::PacketTypes::PacketTypeBleApdu + ) + } + + /// Answer `sw` to an APDU received while another command is in flight. + /// + /// The status word is sent on the intruder's own transport, taken from the + /// received frame, and nothing owned by the in-flight command is touched: + /// `apdu_buffer`, `apdu_type`, `rx`, `rx_length`, `tx`, `tx_length` and + /// `event_pending` are all left as they were. This deliberately bypasses + /// [`Comm::reply`]/`apdu_send`, which would flush any staged response bytes, + /// reset the response state and consume a SEPH event. + fn reject_apdu(&self, sw: StatusWords) { + let sw = sw as u16; + let resp = [(sw >> 8) as u8, sw as u8]; + sys_seph::io_tx(self.io_buffer[0], &resp, resp.len()); + } + /// Wait for the next Command event. Discards received button events. /// /// Like `next_event`, `T` can be any type, an enumeration, or any type diff --git a/ledger_device_sdk/src/nbgl/nbgl_home_and_settings.rs b/ledger_device_sdk/src/nbgl/nbgl_home_and_settings.rs index b72cc4d0..e181605a 100644 --- a/ledger_device_sdk/src/nbgl/nbgl_home_and_settings.rs +++ b/ledger_device_sdk/src/nbgl/nbgl_home_and_settings.rs @@ -293,11 +293,13 @@ impl NbglHomeAndSettings { /// Show the home screen and settings page. /// This function will block until an APDU is received or the user quits the app. - /// DEPRECATED as it constraints to refresh screen for every received APDU. - /// Use `show_and_return` instead. /// # Arguments /// * `_comm` - Mutable reference to Comm. #[cfg(feature = "io_new")] + #[deprecated( + since = "1.37.0", + note = "blocking on an APDU forces the home screen to be refreshed for every received APDU; use `show_and_return` instead" + )] pub fn show, const N: usize>( &mut self, _comm: &mut crate::io::Comm, @@ -310,9 +312,11 @@ impl NbglHomeAndSettings { /// Show the home screen and settings page. /// This function will block until an APDU is received or the user quits the app. - /// DEPRECATED as it constraints to refresh screen for every received APDU. - /// Use `show_and_return` instead. #[cfg(not(feature = "io_new"))] + #[deprecated( + since = "1.37.0", + note = "blocking on an APDU forces the home screen to be refreshed for every received APDU; use `show_and_return` instead" + )] pub fn show>(&mut self) -> Event where Reply: From<>::Error>, From 3814467c7d680aa0e268d09d4a4cecb294ed6fbf Mon Sep 17 00:00:00 2001 From: GroM Date: Mon, 24 Aug 2026 16:43:04 +0200 Subject: [PATCH 4/4] io_new: answer double APDUs on the spot, keep handling BOLOS ones Mirror the io_legacy change in the io_new backend, and fix three behaviours of the double-APDU rejection there. `Comm::pending_apdu` was used as the trigger, but it means "an APDU was decoded ahead and not consumed yet", not "a command is being processed". Rejecting on it deferred the answer to the *next* polling iteration, so a screen completing in between discarded the intruder with no response at all: `CommandResponse::send` clears `pending_apdu` unconditionally. Add `Comm::apdu_in_progress`, set when a command is handed to the application and cleared when the application replies, and reject on the iteration that detects the APDU. The `pending_apdu` branch is kept for the case where no command is in flight and the displayed screen does not exit on APDU: answering there is what lets the polling loop, and so the screen, keep running. Stop gating the inline BOLOS (CLA 0xB0) handling behind `stack_usage`. Default builds set `pending_apdu` for those APDUs and then answered CmdNotAccepted, whereas `next_command` dispatches them unconditionally, so OS level requests failed while a screen was displayed. Save and restore `apdu_in_progress` around the inline call, as the BOLOS reply goes through `begin_response().send()` and would otherwise be taken for the reply to the command still being processed. Answer `DecodedEventType::ApduError` instead of ignoring it: a malformed APDU arriving during a screen used to fall into the catch-all arm and get no status word, leaving the host waiting, while `next_command` and io_legacy both reply BadLen. Reject through a new `Comm::reject_apdu`, which transmits the status word on the transport the rejected APDU arrived on and stages nothing into the shared buffer. `decode_apdu` overwrites `apdu_type` before the rejection is decided, so it is captured on entry and restored on every path that does not deliver the APDU to the application; otherwise the in-flight command's response was sent on the intruder's channel. This also removes the `Reply(StatusWords::CmdNotAccepted as u16)` cast and the call back through `reply_status_impl`, which re-resolved the erased global to build a second `&mut Comm` aliasing the one already in hand. Co-Authored-By: Claude Opus 5 (1M context) --- ledger_device_sdk/CHANGELOG.md | 14 ++++++ ledger_device_sdk/src/io_new.rs | 23 +++++++++ ledger_device_sdk/src/io_new/callbacks.rs | 59 ++++++++++++++++++----- 3 files changed, 84 insertions(+), 12 deletions(-) diff --git a/ledger_device_sdk/CHANGELOG.md b/ledger_device_sdk/CHANGELOG.md index 517cdc5c..89d7edd3 100644 --- a/ledger_device_sdk/CHANGELOG.md +++ b/ledger_device_sdk/CHANGELOG.md @@ -13,6 +13,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 of being queued or silently dropped. Note that adding a variant to the public `StatusWords` enum breaks downstream exhaustive `match` expressions over it. +### Fixed +- `io_new`: BOLOS internal APDUs (CLA 0xB0) arriving while an NBGL screen is + displayed are handled inline again instead of being answered + `CmdNotAccepted`. That handling was gated behind the `stack_usage` feature, so + default builds rejected OS level requests that `next_command` answers. +- `io_new`: a double APDU is now answered on the polling iteration that detects + it. It used to be answered on the next one, so a screen completing in between + discarded it with no response at all, leaving the host waiting. +- `io_new`: a malformed APDU received while a screen is displayed is answered + `BadLen` instead of being ignored, matching `next_command` and `io_legacy`. +- `io_new`: rejecting an APDU no longer leaves `apdu_type` overwritten with the + rejected APDU's transport, which made the in-flight command reply on the + wrong channel. + ### Deprecated - `NbglHomeAndSettings::show()` is now marked with the `#[deprecated]` attribute, matching what its documentation already stated. Use `show_and_return()` diff --git a/ledger_device_sdk/src/io_new.rs b/ledger_device_sdk/src/io_new.rs index 675d5ae8..55ab26d7 100644 --- a/ledger_device_sdk/src/io_new.rs +++ b/ledger_device_sdk/src/io_new.rs @@ -147,6 +147,10 @@ pub struct Comm { pending_header: ApduHeader, pending_offset: usize, pending_length: usize, + /// True from the moment a command is handed to the application until the + /// application replies to it. This is what makes a second incoming APDU a + /// *double* APDU rather than the normal way of receiving work. + apdu_in_progress: bool, } impl Comm { @@ -166,9 +170,23 @@ impl Comm { }, pending_offset: 0, pending_length: 0, + apdu_in_progress: false, } } + /// Answer `sw` to an APDU that cannot be delivered to the application, on + /// the transport it arrived on. + /// + /// This deliberately bypasses [`Comm::begin_response`]: it must stage + /// nothing into the shared buffer and must leave `apdu_type`, + /// `pending_apdu` and `apdu_in_progress` alone, as they belong to the + /// command that is still being processed. + pub(crate) fn reject_apdu>(&self, packet_type: u8, sw: T) { + let sw: u16 = sw.into().0; + let resp = sw.to_be_bytes(); + let _ = sys_seph::io_tx(packet_type, resp.as_ref(), resp.len()); + } + pub(crate) fn nbgl_register_comm(&mut self) { // Register NBGL callbacks if not already set and record current Comm singleton. callbacks::set_comm::(self); @@ -252,6 +270,9 @@ impl Comm { continue; } } + // The command is about to be handed to the application: any + // APDU arriving from now until the reply is a double APDU. + self.apdu_in_progress = true; return Command::new(self, header, offset, length); } // Explicitly convert ApduError -> StatusWords so Into is resolved @@ -411,6 +432,8 @@ impl<'a, const N: usize> CommandResponse<'a, N> { // Clear the pending APDU state after sending a reply, so the next // call to try_next_event will fetch a new event from io_rx. self.comm.pending_apdu = false; + // Replying completes the current command. + self.comm.apdu_in_progress = false; Ok(self.comm) } diff --git a/ledger_device_sdk/src/io_new/callbacks.rs b/ledger_device_sdk/src/io_new/callbacks.rs index 78249f2c..46e6e2fe 100644 --- a/ledger_device_sdk/src/io_new/callbacks.rs +++ b/ledger_device_sdk/src/io_new/callbacks.rs @@ -5,7 +5,6 @@ use crate::io_legacy::{ApduHeader, Reply, StatusWords}; -#[cfg(feature = "stack_usage")] use super::bolos::handle_bolos_apdu; use super::{Comm, DecodedEventType}; @@ -67,38 +66,74 @@ fn panic_reply_impl(reply: Reply) { // Implementation wrappers specialized per const N. +/// Fetch and process one event while an NBGL screen is displayed, and report +/// whether it was an APDU command the caller should leave the screen for. +/// +/// This is called from `ux_sync_wait` both while the application is idle (an +/// incoming APDU is then the normal way of receiving work) and while it is +/// processing a command (an incoming APDU is then a double APDU). pub(super) fn next_event_ahead_impl() -> bool { let comm = unsafe { get_comm::() }; - // If there's already a pending APDU, return Cmd not accepted error status + + // Decoding an APDU overwrites `apdu_type` with the transport it arrived on. + // Anything handled or rejected below is not the command the application is + // working on, so its transport is restored before returning; otherwise the + // in-flight command's response would go out on the intruder's channel. + let in_flight_apdu_type = comm.apdu_type; + + // An APDU detected on an earlier iteration that nobody consumed means the + // displayed screen does not exit on APDU. Answer it, so that polling — and + // therefore the screen itself — keeps running. No command can be in flight + // here, as one would have been rejected on the spot below. if comm.pending_apdu { - reply_status_impl::(Reply(StatusWords::CmdNotAccepted as u16)); + comm.pending_apdu = false; + comm.reject_apdu(in_flight_apdu_type, StatusWords::CmdNotAccepted); return false; } + match comm.next_event().into_type() { DecodedEventType::Apdu { header, offset, length, } => { - // Handle BOLOS internal APDUs (CLA = 0xB0) inline so they don't - // block the ux_sync_wait loop. Without this, a BOLOS APDU arriving - // during an NBGL screen (e.g. stack consumption measurement) would - // set pending_apdu=true and never be consumed when exit_on_apdu=false, - // causing an infinite loop. - #[cfg(feature = "stack_usage")] + // BOLOS internal APDUs (CLA = 0xB0) are answered inline whatever + // the state, the way `next_command` does, so that OS level requests + // keep working while a screen is displayed. if header.cla == 0xB0 { + let in_progress = comm.apdu_in_progress; handle_bolos_apdu::(comm, header.ins, header.p1, header.p2); + // The BOLOS reply must not be taken for the reply to the + // command the application is still processing. + comm.apdu_in_progress = in_progress; + comm.apdu_type = in_flight_apdu_type; + return false; + } + // An APDU arriving while a command is still being processed is a + // double APDU. Answer it on this very iteration: deferring to the + // next one loses it entirely if the screen completes in between. + if comm.apdu_in_progress { + let intruder_apdu_type = comm.apdu_type; + comm.reject_apdu(intruder_apdu_type, StatusWords::CmdNotAccepted); + comm.apdu_type = in_flight_apdu_type; return false; } comm.pending_apdu = true; comm.pending_header = header; comm.pending_offset = offset; comm.pending_length = length; - return true; + true + } + // Answer malformed APDUs instead of leaving the host without a status + // word, as `next_command` does outside of screens. + DecodedEventType::ApduError(e) => { + let intruder_apdu_type = comm.apdu_type; + comm.reject_apdu(intruder_apdu_type, StatusWords::from(e)); + comm.apdu_type = in_flight_apdu_type; + false } - _ => {} + _ => false, } - false } pub(super) fn fetch_apdu_header_impl() -> Option {