Merge pull request #529 from LedgerHQ/xch/IO_CONTINUE_RX

IO Continue RX flag and sync RAPDU sending
LedgerHQ · Feb 9, 2024 · 158f76d · 158f76d
2 parents 8dd9234 + e255f0f
commit 158f76d
Show file tree

Hide file tree

Showing 4 changed files with 66 additions and 34 deletions.
diff --git 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/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/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;

diff --git 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;