Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add some changes from Dune #101

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
File renamed without changes.
103 changes: 103 additions & 0 deletions docs/MEMORY_TIPS.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
Debugging Memory Problems with ledger-app-tezos
==============================================

A typical reason for most problems with the app is lack of
memory. Indeed, the stack and the global variables share a small
read-write memory space. The more variables, the less space for the
stack. When the stack overflows, variables are corrupted, causing a
failure of the application.

The current `Makefile` uses:

```
DEFINES += HAVE_BOLOS_APP_STACK_CANARY
```

With this line, BOLOS introduces a special variable, a canary, at the
end of the stack, and it checks whether it is not modified after every
message. If corruption is detected, the application exits (instead of
freezing later).

If you experiment this, you should create a bug report. You can also
try the following tips to find the problem.

Adding Debug Support
--------------------

First, you will need to install a new firmware on your Ledger. Follow
the documentation for that, to get the `PRINTF` feature working (you
will need [`usbtool`](https://ledger.readthedocs.io/en/latest/userspace/debugging.html#console-printing) on your computer too).

Then, modify the `Makefile` to set the `DEBUG` variable to `1`. `make
clean && make`, and your application can use the `PRINTF` macro to
send information to the computer while running.

You can use `make watch` to start watching the USB log to see the
messages sent by `PRINTF`.

Printing Stack Size
-------------------

Go to your copy of `nanos-secure-sdk`, and use the patch
`nanos-secure-sdk.patch` in the `docs/` directory of
`ledger-app-tezos`.

```
patch -p1 < ../docs/nanos-secure-sdk.patch
```

Now, in the `Makefile`, set the variable `DEBUGMEM` to `1`. `make
clean && make` and your application can use the
`PRINTF_STACK_SIZE("fun_name")` macro.

This macro prints (to the computer USB log) the remaining stack size
at the beginning of a function, once all the space for the variables
in the function has been reserved. Some functions can make a big use
of stack, for example Blake2b functions are expensive (around 400
bytes needed for `b2b_compress`).

Note that `PRINTF_STACK_SIZE` itself has a cost on stack, so, if you
reduce the stack usage, you may still need to remove
`PRINTF_STACK_SIZE` for the application to perform correctly.

Fixing Stack Problems
---------------------

Once you have discovered which stack overflows, you will need to find
a way to reduce the stack usage of some of the functions.

For that, here are a few tips and info:

1. The compiler performs aggressive inlining of functions. Sometimes,
the frame reserved for a function is bigger than it should be, because
it contains the frames of some inline functions. Inlining may also on
the contrary reduce stack frames. So, you will need to investigate by
using the attributes `inline` (to force inlining) and
`__attribute__ ((noinline))` (to prevent inlining).

2. To know which functions are inlined, and the size of the stack
frame of a function, you will need to look at the generated assembly
code. For that, use `make -n` to see how the compiler is called, and
add the `-S` flag to print the assembly code (you will also need to
change the target of `-o`). For example:

```
ledger-env/clang-arm-fropi/bin/clang -c -gdwarf-2 -gstrict-dwarf -O3 -Os -I/usr/include -fomit-frame-pointer -mcpu=cortex-m0 -mthumb -fno-common -mtune=cortex-m0 -mlittle-endian -std=gnu99 -Werror=int-to-pointer-cast -Wall -Wextra -fdata-sections -ffunction-sections -funsigned-char -fshort-enums -mno-unaligned-access -Wno-unused-parameter -Wno-duplicate-decl-specifier -fropi --target=armv6m-none-eabi -fno-jump-tables -O3 -Os -Wall -Wextra -DST31 -Dgcc -D__IO=volatile -DOS_IO_SEPROXYHAL -DHAVE_BAGL -DHAVE_SPRINTF -DHAVE_IO_USB -DHAVE_L4_USBLIB -DIO_USB_MAX_ENDPOINTS=6 -DIO_HID_EP_LENGTH=64 -DHAVE_USB_APDU -DVERSION=\"0.3.1-20190824\" -DCOMMIT=\"7c3880c3\" -DAPPVERSION_M=0 -DAPPVERSION_N=3 -DAPPVERSION_P=1 -DAPPVERSION_D=\"20190824\" -DHAVE_U2F -DHAVE_IO_U2F -DU2F_PROXY_MAGIC=\"XTZ\" -DUSB_SEGMENT_SIZE=64 -DBLE_SEGMENT_SIZE=32 -DHAVE_BOLOS_APP_STACK_CANARY -DIO_SEPROXYHAL_BUFFER_SIZE_B=128 -DHAVE_PATCHED_BOLOS -DHAVE_PRINTF -DPRINTF=screen_printf -I$BOLOS_SDK/lib_stusb/ -I$BOLOS_SDK/lib_stusb/STM32_USB_Device_Library/Class/CCID/inc/ -I$BOLOS_SDK/lib_stusb/STM32_USB_Device_Library/Class/CCID/inc/ -I$BOLOS_SDK/lib_stusb/STM32_USB_Device_Library/Class/CCID/inc/ -I$BOLOS_SDK/lib_stusb/STM32_USB_Device_Library/Class/CCID/inc/ -I$BOLOS_SDK/lib_stusb/STM32_USB_Device_Library/Class/HID/Inc/ -I$BOLOS_SDK/lib_stusb/STM32_USB_Device_Library/Core/Inc/ -I$BOLOS_SDK/lib_stusb/STM32_USB_Device_Library/Core/Inc/ -I$BOLOS_SDK/lib_stusb/STM32_USB_Device_Library/Core/Inc/ -I$BOLOS_SDK/lib_stusb/STM32_USB_Device_Library/Core/Inc/ -I$BOLOS_SDK/lib_stusb/STM32_USB_Device_Library/Core/Inc/ -I$BOLOS_SDK/lib_stusb_impl/ -I$BOLOS_SDK/lib_stusb_impl/ -I$BOLOS_SDK/lib_stusb_impl/ -I$BOLOS_SDK/lib_u2f/include/ -I$BOLOS_SDK/lib_u2f/include/ -I$BOLOS_SDK/lib_u2f/include/ -I$BOLOS_SDK/lib_u2f/include/ -I$BOLOS_SDK/lib_u2f/include/ -Iinclude -I$BOLOS_SDK/include -I$BOLOS_SDK/include/arm -Isrc/ -S -o code.asm src/parse_operations.c
```

will print in file `code.asm` the generate assembly for
`parse_operations.c` (note that we modified `-DCOMMIT` and `-DUNUSED`
to prevent some problems with characters `*` and `()`).

To understand what is happending, you need to know:

* every function has a line with a tag `%function`. If it is does not
have such a line, then the function has been completely inlined in
other functions

* there are annotations `.cfi_def_cfa_offset` that give the size of
the frame for a function at any given point.

3. Finally, since the stack shares memory with global variables,
another solution to solve stack issues is to reduce the size of global
variables.
File renamed without changes.
File renamed without changes.
File renamed without changes.
38 changes: 38 additions & 0 deletions docs/nanos-secure-sdk.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
diff --git a/src/os_io_seproxyhal.c b/src/os_io_seproxyhal.c
index 36171b6..2f75f4e 100644
--- a/src/os_io_seproxyhal.c
+++ b/src/os_io_seproxyhal.c
@@ -917,15 +917,30 @@ unsigned int os_io_seproxyhal_get_app_name_and_version(void) {
}


-unsigned short io_exchange(unsigned char channel, unsigned short tx_len) {
- unsigned short rx_len;
-
+void bolos_check_canary()
+{
#ifdef HAVE_BOLOS_APP_STACK_CANARY
// behavior upon detected stack overflow is to reset the SE
if (app_stack_canary != APP_STACK_CANARY_MAGIC) {
io_seproxyhal_se_reset();
}
#endif // HAVE_BOLOS_APP_STACK_CANARY
+}
+
+
+int bolos_canary_diff()
+{ return (app_stack_canary - APP_STACK_CANARY_MAGIC); }
+
+int bolos_stack_size()
+{
+ char x;
+ return &x - (char*)(&app_stack_canary);
+}
+
+unsigned short io_exchange(unsigned char channel, unsigned short tx_len) {
+ unsigned short rx_len;
+
+ bolos_check_canary();

#ifdef DEBUG_APDU
if ((channel&~(IO_FLAGS)) == CHANNEL_APDU) {
17 changes: 4 additions & 13 deletions src/apdu_sign.c
Original file line number Diff line number Diff line change
@@ -517,7 +517,10 @@ static uint8_t get_magic_byte_or_throw(uint8_t const *const buff, size_t const b
}
}

static size_t handle_apdu(bool const enable_hashing, bool const enable_parsing, uint8_t const instruction) {
size_t handle_apdu_sign(uint8_t const instruction) {
bool const enable_hashing = instruction != INS_SIGN_UNSAFE;
bool const enable_parsing = enable_hashing;

uint8_t *const buff = &G_io_apdu_buffer[OFFSET_CDATA];
uint8_t const p1 = READ_UNALIGNED_BIG_ENDIAN(uint8_t, &G_io_apdu_buffer[OFFSET_P1]);
uint8_t const buff_size = READ_UNALIGNED_BIG_ENDIAN(uint8_t, &G_io_apdu_buffer[OFFSET_LC]);
@@ -608,18 +611,6 @@ static size_t handle_apdu(bool const enable_hashing, bool const enable_parsing,
}
}

size_t handle_apdu_sign(uint8_t instruction) {
bool const enable_hashing = instruction != INS_SIGN_UNSAFE;
bool const enable_parsing = enable_hashing;
return handle_apdu(enable_hashing, enable_parsing, instruction);
}

size_t handle_apdu_sign_with_hash(uint8_t instruction) {
bool const enable_hashing = true;
bool const enable_parsing = true;
return handle_apdu(enable_hashing, enable_parsing, instruction);
}

static int perform_signature(bool const on_hash, bool const send_hash) {
# ifdef BAKING_APP
write_high_water_mark(&G.parsed_baking_data);
2 changes: 1 addition & 1 deletion src/main.c
Original file line number Diff line number Diff line change
@@ -18,7 +18,7 @@ void app_main(void) {
global.handlers[APDU_INS(INS_PROMPT_PUBLIC_KEY)] = handle_apdu_get_public_key;
global.handlers[APDU_INS(INS_SIGN)] = handle_apdu_sign;
global.handlers[APDU_INS(INS_GIT)] = handle_apdu_git;
global.handlers[APDU_INS(INS_SIGN_WITH_HASH)] = handle_apdu_sign_with_hash;
global.handlers[APDU_INS(INS_SIGN_WITH_HASH)] = handle_apdu_sign;
#ifdef BAKING_APP
global.handlers[APDU_INS(INS_AUTHORIZE_BAKING)] = handle_apdu_get_public_key;
global.handlers[APDU_INS(INS_RESET)] = handle_apdu_reset;