From aaded6ed5bc4afa90da423692552a8169fd36eaf Mon Sep 17 00:00:00 2001 From: Xavier Chapron Date: Wed, 7 Feb 2024 16:29:36 +0100 Subject: [PATCH 1/2] os_io_seproxyhal: Introduce new IO_RECEIVE_DATA flag for io_exchange() --- include/os_io.h | 3 +- src/os_io_seproxyhal.c | 81 +++++++++++++++++++++++++----------------- 2 files changed, 51 insertions(+), 33 deletions(-) diff --git a/include/os_io.h b/include/os_io.h index 8cc98900c..0ebb8eb19 100644 --- a/include/os_io.h +++ b/include/os_io.h @@ -37,7 +37,8 @@ extern unsigned char G_io_apdu_buffer[IO_APDU_BUFFER_SIZE]; #define IO_RETURN_AFTER_TX 0x20 #define IO_ASYNCH_REPLY 0x10 // avoid apdu state reset if tx_len == 0 when we're expected to reply #define IO_FINISHED 0x08 // inter task communication value -#define IO_FLAGS 0xF8 +#define IO_CONTINUE_RX 0x04 +#define IO_FLAGS 0xFC unsigned short io_exchange(unsigned char channel_and_flags, unsigned short tx_len); typedef enum { diff --git a/src/os_io_seproxyhal.c b/src/os_io_seproxyhal.c index c0c82219d..757be9327 100644 --- a/src/os_io_seproxyhal.c +++ b/src/os_io_seproxyhal.c @@ -1487,47 +1487,45 @@ unsigned short io_exchange(unsigned char channel, unsigned short tx_len) } } - if (!(channel & IO_ASYNCH_REPLY)) { - // already received the data of the apdu when received the whole apdu - if ((channel & (CHANNEL_APDU | IO_RECEIVE_DATA)) - == (CHANNEL_APDU | IO_RECEIVE_DATA)) { - // return apdu data - header - return G_io_app.apdu_length - 5; + // When IO_CONTINUE_RX is used we don't reset potentially already received APDU. + // Instead we directly process them. + // Use case is: + // - First APDU received (call to io_exchange()) + // - UX waiting for user approval + // - First APDU response sent (call to io_exchange() with flag IO_RETURN_AFTER_TX) + // - UX with transient message like ("Transaction signed") + // This need to loop on os_io_seph_recv_and_process() to process tick and UX + // events, but a second APDU can be received here too. + // - Then a call to io_exchange() with flag IO_CONTINUE_RX will allow processing + // the APDU now that the app is ready. + // + // Note that a received APDU will be cleared out (reset of G_io_app.apdu_state / + // G_io_app.apdu_media / G_io_app.apdu_length) during the APDU response sending. + // Therefore there is no risk to process an APDU twice. + if (!(channel & IO_CONTINUE_RX)) { + if (!(channel & IO_ASYNCH_REPLY)) { + // already received the data of the apdu when received the whole apdu + if ((channel & (CHANNEL_APDU | IO_RECEIVE_DATA)) + == (CHANNEL_APDU | IO_RECEIVE_DATA)) { + // return apdu data - header + return G_io_app.apdu_length - 5; + } + + // reply has ended, proceed to next apdu reception (reset status only after + // asynch reply) + G_io_app.apdu_state = APDU_IDLE; + G_io_app.apdu_media = IO_APDU_MEDIA_NONE; } - // reply has ended, proceed to next apdu reception (reset status only after asynch - // reply) - G_io_app.apdu_state = APDU_IDLE; - G_io_app.apdu_media = IO_APDU_MEDIA_NONE; + // reset the received apdu length + G_io_app.apdu_length = 0; } - // reset the received apdu length - G_io_app.apdu_length = 0; - // ensure ready to receive an event (after an apdu processing with asynch flag, it may // occur if the channel is not correctly managed) // until a new whole CAPDU is received for (;;) { - io_seproxyhal_general_status(); - // wait until a SPI packet is available - // NOTE: on ST31, dual wait ISO & RF (ISO instead of SPI) - rx_len = io_seproxyhal_spi_recv( - G_io_seproxyhal_spi_buffer, sizeof(G_io_seproxyhal_spi_buffer), 0); - - // can't process split TLV, continue - if (rx_len < 3 - || rx_len - != U2(G_io_seproxyhal_spi_buffer[1], G_io_seproxyhal_spi_buffer[2]) - + 3U) { - LOG("invalid TLV format\n"); - G_io_app.apdu_state = APDU_IDLE; - G_io_app.apdu_length = 0; - continue; - } - - io_seproxyhal_handle_event(); - // An apdu has been received asynchronously. if (G_io_app.apdu_state != APDU_IDLE && G_io_app.apdu_length > 0) { // for Bolos UX and apps, answer SWO_SEC_PIN_15 as soon as PIN has been set and @@ -1550,6 +1548,25 @@ unsigned short io_exchange(unsigned char channel, unsigned short tx_len) return G_io_app.apdu_length; } + + io_seproxyhal_general_status(); + // wait until a SPI packet is available + // NOTE: on ST31, dual wait ISO & RF (ISO instead of SPI) + rx_len = io_seproxyhal_spi_recv( + G_io_seproxyhal_spi_buffer, sizeof(G_io_seproxyhal_spi_buffer), 0); + + // can't process split TLV, continue + if (rx_len < 3 + || rx_len + != U2(G_io_seproxyhal_spi_buffer[1], G_io_seproxyhal_spi_buffer[2]) + + 3U) { + LOG("invalid TLV format\n"); + G_io_app.apdu_state = APDU_IDLE; + G_io_app.apdu_length = 0; + continue; + } + + io_seproxyhal_handle_event(); } break; From e255f0ff7e6bfc0e2cd9dc2d008a28ace81c3641 Mon Sep 17 00:00:00 2001 From: Xavier Chapron Date: Wed, 7 Feb 2024 17:44:11 +0100 Subject: [PATCH 2/2] lib_standard_app/io.c: Send RAPDU immediately and use IO_CONTINUE_RX flag --- Makefile.standard_app | 6 ++++++ lib_standard_app/io.c | 10 +++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/Makefile.standard_app b/Makefile.standard_app index a880033f0..a0be83a3d 100644 --- a/Makefile.standard_app +++ b/Makefile.standard_app @@ -124,6 +124,12 @@ ifneq ($(DISABLE_STANDARD_APP_FILES), 1) SDK_SOURCE_PATH += lib_standard_app endif +ifneq ($(DISABLE_STANDARD_APP_SYNC_RAPDU), 1) +ifneq ($(TARGET_NAME),TARGET_NANOS) +DEFINES += STANDARD_APP_SYNC_RAPDU +endif +endif + ##################################################################### # NBGL # ##################################################################### diff --git a/lib_standard_app/io.c b/lib_standard_app/io.c index 57016ea75..27581c4fb 100644 --- a/lib_standard_app/io.c +++ b/lib_standard_app/io.c @@ -134,8 +134,8 @@ WEAK int io_recv_command() switch (G_io_state) { case READY: + ret = io_exchange(CHANNEL_APDU | IO_CONTINUE_RX, G_output_len); G_io_state = RECEIVED; - ret = io_exchange(CHANNEL_APDU, G_output_len); break; case RECEIVED: G_io_state = WAITING; @@ -198,9 +198,17 @@ WEAK int io_send_response_buffers(const buffer_t *rdatalist, size_t count, uint1 ret = -1; break; case RECEIVED: +#ifdef STANDARD_APP_SYNC_RAPDU + // Send synchronously the APDU response. + // This is needed to send the response before displaying synchronous + // status message on the screen. + // This is not always done to spare the RAM (stack) on LNS. + __attribute__((fallthrough)); +#else G_io_state = READY; ret = 0; break; +#endif case WAITING: ret = io_exchange(CHANNEL_APDU | IO_RETURN_AFTER_TX, G_output_len); G_output_len = 0;