Ensure temporary pointers allocated in G_io_apdu_buffer are memory aligned

Memory access of pointers to data types larger than one byte need to be aligned
to 32bit words (or 16bit for shorts), 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
building with the SDK's Makefile.standard_app as of 5542f4b, 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 speculos does not enforce memory alignment, which is why the issue did
not show on the emulator.

Two instances of unaligned memory access existed:
- in review_entries_launch_use_case_review for a nbgl_contentTagValueList_t *
  pointer on NBGL devices Flex and Stax, which caused these devices to freeze
  as of 5542f4b on memset and memcpy to the unaligned pointers.
- in on_transaction_approved for a cx_ecfp_256_public_key_t * pointer on all
  devices, which was not harmful though as only uint8_t data was accessed, which
  does not need to be aligned.

Author: danimoh

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_