From 28d282101e8101be43cbe840b4469b254bc10f0e Mon Sep 17 00:00:00 2001 From: Ondrej Mikle Date: Tue, 8 Feb 2022 16:54:32 +0100 Subject: [PATCH] fix(legacy): cleanup of undesired states where USB processing should not happen --- legacy/common.c | 13 +++++++++++- legacy/firmware/.changelog.d/2107.fixed | 1 + legacy/firmware/config.c | 2 +- legacy/firmware/protect.c | 2 +- legacy/firmware/recovery.c | 2 +- legacy/firmware/trezor.c | 6 +++--- legacy/firmware/udp.c | 13 +++++++++--- legacy/firmware/usb.c | 2 +- legacy/firmware/usb.h | 28 ++++++++++++++++++++++++- 9 files changed, 57 insertions(+), 12 deletions(-) create mode 100644 legacy/firmware/.changelog.d/2107.fixed diff --git a/legacy/common.c b/legacy/common.c index 5e64c89b3..77fe4b85a 100644 --- a/legacy/common.c +++ b/legacy/common.c @@ -19,6 +19,7 @@ #include "common.h" #include +#include #include "bitmaps.h" #include "firmware/usb.h" #include "hmac_drbg.h" @@ -83,7 +84,17 @@ void __assert_func(const char *file, int line, const char *func, } #endif -void hal_delay(uint32_t ms) { usbSleep(ms); } +void hal_delay(uint32_t ms) { +#if EMULATOR + usleep(ms * 1000); +#else + uint32_t start = timer_ms(); + + while ((timer_ms() - start) < ms) { + asm("nop"); + } +#endif +} void drbg_init() { uint8_t entropy[48] = {0}; diff --git a/legacy/firmware/.changelog.d/2107.fixed b/legacy/firmware/.changelog.d/2107.fixed new file mode 100644 index 000000000..0f0cf66f9 --- /dev/null +++ b/legacy/firmware/.changelog.d/2107.fixed @@ -0,0 +1 @@ +Fix legacy technical debt in USB handling (readability and FSM unwanted states). diff --git a/legacy/firmware/config.c b/legacy/firmware/config.c index 726cc3d5c..2eb0dfbb9 100644 --- a/legacy/firmware/config.c +++ b/legacy/firmware/config.c @@ -564,7 +564,7 @@ void config_setHomescreen(const uint8_t *data, uint32_t size) { } static void get_root_node_callback(uint32_t iter, uint32_t total) { - usbSleep(1); + waitAndProcessUSBRequests(1); layoutProgress(_("Waking up"), 1000 * iter / total); } diff --git a/legacy/firmware/protect.c b/legacy/firmware/protect.c index 569d1f459..e70f7e179 100644 --- a/legacy/firmware/protect.c +++ b/legacy/firmware/protect.c @@ -64,7 +64,7 @@ bool protectButton(ButtonRequestType type, bool confirm_only) { // button acked - check buttons if (acked) { - usbSleep(5); + waitAndProcessUSBRequests(5); buttonUpdate(); if (button.YesUp) { result = true; diff --git a/legacy/firmware/recovery.c b/legacy/firmware/recovery.c index db708915d..48dad99d8 100644 --- a/legacy/firmware/recovery.c +++ b/legacy/firmware/recovery.c @@ -421,7 +421,7 @@ static void recovery_digit(const char digit) { oledInvert(x + 1, y, x + 62, y + 9); oledRefresh(); usbTiny(1); - usbSleep(250); + waitAndProcessUSBRequests(250); usbTiny(0); /* index of the chosen word */ diff --git a/legacy/firmware/trezor.c b/legacy/firmware/trezor.c index d1fe24e34..b59a89327 100644 --- a/legacy/firmware/trezor.c +++ b/legacy/firmware/trezor.c @@ -77,13 +77,13 @@ void check_lock_screen(void) { // wait until NoButton is released usbTiny(1); do { - usbSleep(5); + waitAndProcessUSBRequests(5); buttonUpdate(); } while (!button.NoUp); // wait for confirmation/cancellation of the dialog do { - usbSleep(5); + waitAndProcessUSBRequests(5); buttonUpdate(); } while (!button.YesUp && !button.NoUp); usbTiny(0); @@ -180,7 +180,7 @@ int main(void) { usbInit(); for (;;) { #if EMULATOR - usbSleep(10); + waitAndProcessUSBRequests(10); #else usbPoll(); #endif diff --git a/legacy/firmware/udp.c b/legacy/firmware/udp.c index dd56ec934..645c80dce 100644 --- a/legacy/firmware/udp.c +++ b/legacy/firmware/udp.c @@ -18,6 +18,7 @@ */ #include +#include #include "usb.h" @@ -35,7 +36,7 @@ void usbInit(void) { emulatorSocketInit(); } #define _ISDBG ('n') #endif -void usbSleep(uint32_t millis) { +void waitAndProcessUSBRequests(uint32_t millis) { emulatorPoll(); static uint8_t buffer[USB_PACKET_SIZE]; @@ -63,7 +64,7 @@ void usbSleep(uint32_t millis) { #endif } -void usbPoll(void) { usbSleep(0); } +void usbPoll(void) { waitAndProcessUSBRequests(0); } char usbTiny(char set) { char old = tiny; @@ -71,4 +72,10 @@ char usbTiny(char set) { return old; } -void usbFlush(uint32_t millis) { usbSleep(millis); } +void usbFlush(uint32_t millis) { + const uint8_t *data; + while ((data = msg_out_data()) != NULL) { + emulatorSocketWrite(0, data, USB_PACKET_SIZE); + } + usleep(millis * 1000); +} diff --git a/legacy/firmware/usb.c b/legacy/firmware/usb.c index 8b98e2a36..352e9bb4d 100644 --- a/legacy/firmware/usb.c +++ b/legacy/firmware/usb.c @@ -448,7 +448,7 @@ char usbTiny(char set) { return old; } -void usbSleep(uint32_t millis) { +void waitAndProcessUSBRequests(uint32_t millis) { uint32_t start = timer_ms(); while ((timer_ms() - start) < millis) { diff --git a/legacy/firmware/usb.h b/legacy/firmware/usb.h index 8df36606d..757a691df 100644 --- a/legacy/firmware/usb.h +++ b/legacy/firmware/usb.h @@ -25,8 +25,34 @@ void usbInit(void); void usbPoll(void); void usbReconnect(void); + +/* + * Setting this value to 1 will limit the protobuf messages `usbPoll` and + * `waitAndProcessUSBRequests` can handle to a few defined in `msg_read_tiny`. + * + * Also affects U2F and DebugLink messages. + * + * Setting to 1 is meant to prevent infinite recursion when you need to read a + * message while being called from FSM. + * + * Setting to 0 allows processing all messages. + */ char usbTiny(char set); -void usbSleep(uint32_t millis); + +/* + * This will wait given number of milliseconds for arrival of protobuf message. + * If it arrives, it will service it before returning. + * + * If you call this function from any function that is called from FSM, + * you must use `usbTiny(1)` before and `usbTiny(oldTinyValue)` or `usbTiny(0)` + * after, otherwise there is possibility of stack exhaustion. + */ +void waitAndProcessUSBRequests(uint32_t millis); + +/* + * Flush out any messages still in USB bus FIFO while waiting given number + * of milliseconds. Any incoming USB protobuf messages are not serviced. + */ void usbFlush(uint32_t millis); #endif