Merge pull request LedgerHQ#7 from nimiq/develop

Avoid unaligned memory access in NBGL UIs on Stax and Flex

Makefile

@@ -32,7 +32,7 @@ APPNAME = Nimiq
 # Application version
 APPVERSION_M = 2
 APPVERSION_N = 0
-APPVERSION_P = 0
+APPVERSION_P = 1
 APPVERSION = "$(APPVERSION_M).$(APPVERSION_N).$(APPVERSION_P)"
 
 # Setting to allow building variant applications. For now, there are no variants, only the main Nimiq app.

src/main.c

@@ -179,12 +179,8 @@ void on_transaction_approved() {
         created_staker_signature = true;
 
         // Similarly, overwrite the public key in the signature proof with the ledger account public key as staker, with
-        // G_io_apdu_buffer as temporary buffer again. Check with a compile time assertion that it can fit the temp data
-        _Static_assert(
-            sizeof(cx_ecfp_256_public_key_t) <= sizeof(G_io_apdu_buffer),
-            "G_io_apdu_buffer does not fit public key\n"
-        );
-        cx_ecfp_256_public_key_t *temporary_public_key_pointer = (cx_ecfp_256_public_key_t*) G_io_apdu_buffer;
+        // G_io_apdu_buffer as temporary buffer again.
+        DECLARE_TEMPORARY_G_IO_APDU_BUFFER_POINTER(cx_ecfp_256_public_key_t *, temporary_public_key_pointer);
         GOTO_ON_ERROR(
             cx_ecfp_generate_pair_no_throw(
                 /* curve */ CX_CURVE_Ed25519,

src/nimiq_ux_nbgl.c

@@ -141,15 +141,11 @@ static void review_entries_launch_use_case_review(
     bool use_small_font
 ) {
     // The tagValueList gets copied in nbgl_useCaseReview, therefore it's sufficient to initialize it only in temporary
-    // memory. We use G_io_apdu buffer as temporary buffer, to save some stack space, and check that it can fit the data
-    // with a compile time assertion. Note that the entries are not copied, and we therefore have them in global memory.
-    _Static_assert(
-        sizeof(nbgl_contentTagValueList_t) <= sizeof(G_io_apdu_buffer),
-        "G_io_apdu_buffer can't fit review list\n"
-    );
-    nbgl_contentTagValueList_t *temporary_tag_value_list_pointer = (nbgl_contentTagValueList_t*) G_io_apdu_buffer;
+    // memory. We use G_io_apdu buffer as temporary buffer, to save some stack space. Note that the review entries are
+    // not copied, and we therefore have them in global memory.
+    DECLARE_TEMPORARY_G_IO_APDU_BUFFER_POINTER(nbgl_contentTagValueList_t *, temporary_tag_value_list_pointer);
     // Initialize list with zeroes, including .nbMaxLinesForValue (no limit of lines to display).
-    memset(temporary_tag_value_list_pointer, 0, sizeof(nbgl_contentTagValueList_t));
+    memset(temporary_tag_value_list_pointer, 0, sizeof(*temporary_tag_value_list_pointer));
     temporary_tag_value_list_pointer->wrapping = true; // Prefer wrapping on spaces, e.g. for nicer address formatting.
     temporary_tag_value_list_pointer->smallCaseForValue = use_small_font;
     temporary_tag_value_list_pointer->pairs = review_entries.entries;
@@ -170,7 +166,7 @@ static void review_entries_launch_use_case_review(
     // static variable in nbgl_use_case.c and there's also no non-static method that exposes pointers to it. Instead, we
     // reset the temporary buffer, to make it immediately noticeable via Ragger end-to-end tests if the data was not
     // copied. Use explicit_bzero instead of memset to avoid this being optimized away.
-    explicit_bzero(temporary_tag_value_list_pointer, sizeof(nbgl_contentTagValueList_t));
+    explicit_bzero(temporary_tag_value_list_pointer, sizeof(*temporary_tag_value_list_pointer));
 }
 
 //////////////////////////////////////////////////////////////////////

src/utility_macros.h

@@ -56,7 +56,7 @@
 
 #define STRING_LENGTH_WITH_SUFFIX(length_a, suffix) (length_a - /* string terminator of string a */ 1 + sizeof(suffix))
 
-#define STRUCT_MEMBER_SIZE(struct_type, member) (sizeof(((struct_type *) 0)->member))
+#define STRUCT_MEMBER_SIZE(struct_type, member) (sizeof(((struct_type *) NULL)->member))
 
 /**
  * Copy data of fixed size to a destination of fixed size. This macro performs a static assertion at compile time, that
@@ -73,4 +73,39 @@
         memcpy(destination, source, sizeof(source)); \
     })
 
+#define POINTER_SIZE (sizeof(void *))
+/**
+ * Get the offset of a pointer from 32bit based memory alignment. Memory access of pointers to data types larger than
+ * one byte need to be aligned to 32bit words / POINTER_SIZE (or 16bit for shorts, which we don't handle separately here
+ * and always use 32bit alignment), as required by Ledger Nano S's hardware, and for other devices by the compiler flags
+ * (presumably, as other devices do in fact support non-aligned memory access, however without memory alignment, the app
+ * froze on the unaligned memset in review_entries_launch_use_case_review since using the SDK's Makefile.standard_app
+ * for building as of 5542f4b6, but not before that change. I did not further investigate, which exact flag introduced
+ * the issue, or whether maybe the accessed pointer was 32bit aligned before that change by coincidence). Note that the
+ * speculos emulator does not enforce any memory alignment, which is why related issues do not show there.
+ * In any case, it's a good idea to conform to memory alignment.
+ */
+#define POINTER_MEMORY_ALIGNMENT_OFFSET(pointer) (((uintptr_t) pointer) % POINTER_SIZE)
+
+/**
+ * Declare a pointer of given type within G_io_apdu_buffer for temporary use, while G_io_apdu_buffer is not otherwise
+ * used or accessed. The pointer is ensured to be memory aligned, see POINTER_MEMORY_ALIGNMENT_OFFSET, and its type is
+ * checked to fit G_io_apdu_buffer.
+ */
+#define DECLARE_TEMPORARY_G_IO_APDU_BUFFER_POINTER(pointer_type, variable_name) \
+    _Static_assert( \
+        /* The memory alignment offset can not be statically computed at compile time, therefore we assume for the */ \
+        /* static assertion that the largest padding POINTER_SIZE - 1 has to be applied. */ \
+        sizeof(*((pointer_type) NULL)) <= sizeof(G_io_apdu_buffer) - (POINTER_SIZE - 1), \
+        "Pointer type does not fit G_io_apdu_buffer\n" \
+    ); \
+    pointer_type variable_name = (pointer_type) ( \
+        G_io_apdu_buffer + \
+        ( \
+            POINTER_MEMORY_ALIGNMENT_OFFSET(G_io_apdu_buffer) \
+                ? POINTER_SIZE - POINTER_MEMORY_ALIGNMENT_OFFSET(G_io_apdu_buffer) \
+                : 0 \
+        ) \
+    );
+
 #endif // _NIMIQ_UTILITY_MACROS_H_

tests/test_get_name_and_version.py

@@ -26,4 +26,4 @@ def test_get_app_and_version(backend):
     app_name, version = app_name_raw.decode("ascii"), version_raw.decode("ascii")
 
     assert app_name == "Nimiq"
-    assert version == "2.0.0"
+    assert version == "2.0.1"