From 502343779645ca82fe19da9cb6b99dde9cf357d4 Mon Sep 17 00:00:00 2001 From: David Lechner Date: Sat, 19 Jul 2025 17:01:07 -0500 Subject: [PATCH] pbio/sys/storage_data: fix strict aliasing issues Fix strict aliasing issues between block drivers and pbsys storage. Casting a uint8_t* to a struct pointer violates strict aliasing rules, which can lead to hard-to-find bugs. We need to ensure that the memory being pointed to has the same alignment, etc. as the struct it is being cast to. The usual way to do this is to use a union of the struct and the other type we want to use (in this case, uint8_t array). To do this, we need to move the storage data struct definition to a header file that can be included by both pbdrv/block_device and pbsys/storage. Additionally, in block_device_test, the size of the ramdisk block was wrong and was causing the NXT to crash sometime after starting the REPL. It also incorrectly had the .noinit section attribute, but depends on being initialized to zero, so that is removed. Fixes: https://github.com/pybricks/support/issues/2272 Fixes: ce4253aec ("pbio/sys: Move block device read to driver level.") --- lib/pbio/drv/block_device/block_device_ev3.c | 25 ++++--- .../block_device/block_device_flash_stm32.c | 13 ++-- lib/pbio/drv/block_device/block_device_test.c | 26 ++++--- .../block_device/block_device_w25qxx_stm32.c | 13 ++-- lib/pbio/include/pbdrv/block_device.h | 7 +- lib/pbio/platform/nxt/pbdrvconfig.h | 2 - lib/pbio/platform/nxt/pbsysconfig.h | 2 +- lib/pbio/sys/storage.c | 53 +------------- lib/pbio/sys/storage_data.h | 69 +++++++++++++++++++ 9 files changed, 117 insertions(+), 93 deletions(-) create mode 100644 lib/pbio/sys/storage_data.h diff --git a/lib/pbio/drv/block_device/block_device_ev3.c b/lib/pbio/drv/block_device/block_device_ev3.c index 635a06104..72c429361 100644 --- a/lib/pbio/drv/block_device/block_device_ev3.c +++ b/lib/pbio/drv/block_device/block_device_ev3.c @@ -20,13 +20,6 @@ #include #include -#include "../core.h" - -#include -#include -#include -#include - #include #include #include @@ -36,9 +29,15 @@ #include #include -#include "block_device_ev3.h" +#include "../core.h" #include "../drv/gpio/gpio_ev3.h" +#include "../sys/storage_data.h" +#include "block_device_ev3.h" +#include +#include +#include +#include #include #include #include @@ -831,7 +830,11 @@ static struct { * portion of this, up to pbdrv_block_device_get_writable_size() bytes, * gets saved to flash at shutdown. */ - uint8_t data[PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE]; + union { + // ensure that data is properly aligned for pbsys_storage_data_map_t + pbsys_storage_data_map_t data_map; + uint8_t data[PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE]; + }; } ramdisk __attribute__((section(".noinit"), used)); uint32_t pbdrv_block_device_get_writable_size(void) { @@ -840,8 +843,8 @@ uint32_t pbdrv_block_device_get_writable_size(void) { static pbio_error_t pbdrv_block_device_load_err = PBIO_ERROR_FAILED; -pbio_error_t pbdrv_block_device_get_data(uint8_t **data) { - *data = ramdisk.data; +pbio_error_t pbdrv_block_device_get_data(pbsys_storage_data_map_t **data) { + *data = &ramdisk.data_map; // Higher level code can use the ramdisk data if initialization completed // successfully. Otherwise it should reset to factory default data. diff --git a/lib/pbio/drv/block_device/block_device_flash_stm32.c b/lib/pbio/drv/block_device/block_device_flash_stm32.c index 20e3b6220..696bbb9f6 100644 --- a/lib/pbio/drv/block_device/block_device_flash_stm32.c +++ b/lib/pbio/drv/block_device/block_device_flash_stm32.c @@ -10,9 +10,8 @@ #include #include -#include - #include "../core.h" +#include "../sys/storage_data.h" #include @@ -41,7 +40,11 @@ static struct { * portion of this, up to pbdrv_block_device_get_writable_size() bytes, * gets saved to flash at shutdown. */ - uint8_t data[PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE]; + union { + // ensure that data is properly aligned for pbsys_storage_data_map_t + pbsys_storage_data_map_t data_map; + uint8_t data[PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE]; + }; } ramdisk __attribute__((section(".noinit"), used)); const uint32_t header_size = sizeof(ramdisk.saved_size) + sizeof(ramdisk.checksum_complement); @@ -70,8 +73,8 @@ void pbdrv_block_device_init(void) { memcpy(&ramdisk, _pbdrv_block_device_storage_start, size); } -pbio_error_t pbdrv_block_device_get_data(uint8_t **data) { - *data = ramdisk.data; +pbio_error_t pbdrv_block_device_get_data(pbsys_storage_data_map_t **data) { + *data = &ramdisk.data_map; return init_err; } diff --git a/lib/pbio/drv/block_device/block_device_test.c b/lib/pbio/drv/block_device/block_device_test.c index 496aa49e8..572c8eb20 100644 --- a/lib/pbio/drv/block_device/block_device_test.c +++ b/lib/pbio/drv/block_device/block_device_test.c @@ -10,8 +10,9 @@ #include #include -#include +#include "../sys/storage_data.h" +#include #include /** @@ -59,23 +60,20 @@ static const uint8_t _program_data[] = { // version at the right place. FIXME: Move the git version to pybricks build // system, since it isn't actually the micropython git version. #include "genhdr/mpversion.h" -#include + static struct { - uint8_t user_data[PBSYS_CONFIG_STORAGE_USER_DATA_SIZE]; - char stored_firmware_hash[8]; - pbsys_storage_settings_t settings; - uint32_t program_offset; - uint32_t program_size; - uint8_t program_data[sizeof(_program_data)]; -} ramdisk __attribute__((section(".noinit"), used)) = { 0 }; + // ensure that data is properly aligned for pbsys_storage_data_map_t + pbsys_storage_data_map_t data_map; + uint8_t data[PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE]; +} ramdisk; uint32_t pbdrv_block_device_get_writable_size(void) { return 0; } -pbio_error_t pbdrv_block_device_get_data(uint8_t **data) { - *data = (void *)&ramdisk; +pbio_error_t pbdrv_block_device_get_data(pbsys_storage_data_map_t **data) { + *data = &ramdisk.data_map; // Higher level code can use the ramdisk data if initialization completed // successfully. Otherwise it should reset to factory default data. @@ -83,9 +81,9 @@ pbio_error_t pbdrv_block_device_get_data(uint8_t **data) { } void pbdrv_block_device_init(void) { - ramdisk.program_size = sizeof(_program_data); - memcpy(&ramdisk.stored_firmware_hash[0], MICROPY_GIT_HASH, sizeof(ramdisk.stored_firmware_hash)); - memcpy(&ramdisk.program_data[0], _program_data, sizeof(_program_data)); + ramdisk.data_map.slot_info[0].size = sizeof(_program_data); + memcpy(ramdisk.data_map.stored_firmware_hash, MICROPY_GIT_HASH, sizeof(ramdisk.data_map.stored_firmware_hash)); + memcpy(ramdisk.data_map.program_data, _program_data, sizeof(_program_data)); } // Don't store any data in this implementation. diff --git a/lib/pbio/drv/block_device/block_device_w25qxx_stm32.c b/lib/pbio/drv/block_device/block_device_w25qxx_stm32.c index 73f250c97..666140f25 100644 --- a/lib/pbio/drv/block_device/block_device_w25qxx_stm32.c +++ b/lib/pbio/drv/block_device/block_device_w25qxx_stm32.c @@ -13,9 +13,8 @@ #include STM32_HAL_H -#include - #include "../core.h" +#include "../sys/storage_data.h" #include "block_device_w25qxx_stm32.h" #include @@ -36,15 +35,19 @@ static struct { * portion of this, up to pbdrv_block_device_get_writable_size() bytes, * gets saved to flash at shutdown. */ - uint8_t data[PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE]; + union { + // ensure that data is properly aligned for pbsys_storage_data_map_t + pbsys_storage_data_map_t data_map; + uint8_t data[PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE]; + }; } ramdisk __attribute__((section(".noinit"), used)); uint32_t pbdrv_block_device_get_writable_size(void) { return PBDRV_CONFIG_BLOCK_DEVICE_W25QXX_STM32_SIZE - sizeof(ramdisk.saved_size); } -pbio_error_t pbdrv_block_device_get_data(uint8_t **data) { - *data = ramdisk.data; +pbio_error_t pbdrv_block_device_get_data(pbsys_storage_data_map_t **data) { + *data = &ramdisk.data_map; // Higher level code can use the ramdisk data if initialization completed // successfully. Otherwise it should reset to factory default data. diff --git a/lib/pbio/include/pbdrv/block_device.h b/lib/pbio/include/pbdrv/block_device.h index a29e9261e..875dfd871 100644 --- a/lib/pbio/include/pbdrv/block_device.h +++ b/lib/pbio/include/pbdrv/block_device.h @@ -11,9 +11,10 @@ #include +#include "../sys/storage_data.h" + #include #include - #include #if PBDRV_CONFIG_BLOCK_DEVICE @@ -28,7 +29,7 @@ * ::PBIO_ERROR_IO (driver-specific error), though boot * does not proceed if this happens. */ -pbio_error_t pbdrv_block_device_get_data(uint8_t **data); +pbio_error_t pbdrv_block_device_get_data(pbsys_storage_data_map_t **data); /** * Writes the "RAM Disk" to storage. May erase entire disk prior to writing. @@ -57,7 +58,7 @@ uint32_t pbdrv_block_device_get_writable_size(void); #else -static inline pbio_error_t pbdrv_block_device_get_data(uint8_t **data) { +static inline pbio_error_t pbdrv_block_device_get_data(pbsys_storage_data_map_t **data) { return PBIO_ERROR_NOT_SUPPORTED; } diff --git a/lib/pbio/platform/nxt/pbdrvconfig.h b/lib/pbio/platform/nxt/pbdrvconfig.h index 72c635b53..3b8c32877 100644 --- a/lib/pbio/platform/nxt/pbdrvconfig.h +++ b/lib/pbio/platform/nxt/pbdrvconfig.h @@ -11,8 +11,6 @@ #define PBDRV_CONFIG_BLOCK_DEVICE (1) #define PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE (10 * 1024) #define PBDRV_CONFIG_BLOCK_DEVICE_TEST (1) -#define PBDRV_CONFIG_BLOCK_DEVICE_TEST_SIZE (8 * 1024) -#define PBDRV_CONFIG_BLOCK_DEVICE_TEST_SIZE_USER (512) #define PBDRV_CONFIG_BUTTON (1) #define PBDRV_CONFIG_BUTTON_NXT (1) diff --git a/lib/pbio/platform/nxt/pbsysconfig.h b/lib/pbio/platform/nxt/pbsysconfig.h index 65b9060c9..7f5399e01 100644 --- a/lib/pbio/platform/nxt/pbsysconfig.h +++ b/lib/pbio/platform/nxt/pbsysconfig.h @@ -11,7 +11,7 @@ #define PBSYS_CONFIG_MAIN (1) #define PBSYS_CONFIG_STORAGE (1) #define PBSYS_CONFIG_STORAGE_NUM_SLOTS (1) -#define PBSYS_CONFIG_STORAGE_USER_DATA_SIZE (PBDRV_CONFIG_BLOCK_DEVICE_TEST_SIZE_USER) +#define PBSYS_CONFIG_STORAGE_USER_DATA_SIZE (512) #define PBSYS_CONFIG_STATUS_LIGHT (0) #define PBSYS_CONFIG_STATUS_LIGHT_BATTERY (0) #define PBSYS_CONFIG_STATUS_LIGHT_BLUETOOTH (0) diff --git a/lib/pbio/sys/storage.c b/lib/pbio/sys/storage.c index daa13e57b..c1a283008 100644 --- a/lib/pbio/sys/storage.c +++ b/lib/pbio/sys/storage.c @@ -22,62 +22,11 @@ #include "core.h" #include "hmi.h" -/** - * Information about one code slot. - * - * A size of 0 means that this slot is not used. The offset indicates where - * the program is stored in user storage. - * - * Code slots are *not* stored chronologically by slot id. Instead they are - * stored consecutively as they are received, with the newest program last. - * - * If a slot is already in use and a new program should be loaded into it, - * it is deleted by mem-moving any subsequent programs into its place, and - * appending the new program to be last again. Since a user is typically only - * iterating code in one slot, this is therefore usually the last stored - * program. This means memmoves happen very little, only when needed. - * - */ -typedef struct { - uint32_t offset; - uint32_t size; -} pbsys_storage_slot_info_t; - /** * Slot at which incoming program data is currently being received. */ static uint8_t incoming_slot = 0; -/** - * Map of loaded data. All data types are little-endian. - */ -typedef struct { - /** - * End-user read-write accessible data. Everything after this is also - * user-readable but not writable. - */ - uint8_t user_data[PBSYS_CONFIG_STORAGE_USER_DATA_SIZE]; - /** - * First 8 symbols of the git hash of the firmware version used to create - * this data map. If this does not match the version of the running - * firmware, user data will be reset to 0. - */ - char stored_firmware_hash[8]; - /** - * System settings. Settings will be reset to defaults when the firmware - * version changes due to an update. - */ - pbsys_storage_settings_t settings; - /** - * Size and offset info for each slot. - */ - pbsys_storage_slot_info_t slot_info[PBSYS_CONFIG_STORAGE_NUM_SLOTS]; - /** - * Data of the application program (code + heap). - */ - uint8_t program_data[] __attribute__((aligned(sizeof(void *)))); -} pbsys_storage_data_map_t; - /** * Map of loaded data. This is kept in memory between successive runs of the * end user application (like MicroPython). @@ -399,7 +348,7 @@ static pbio_error_t pbsys_storage_deinit_process_thread(pbio_os_state_t *state, */ void pbsys_storage_init(void) { - pbio_error_t err = pbdrv_block_device_get_data((uint8_t **)&map); + pbio_error_t err = pbdrv_block_device_get_data(&map); // Test that storage successfully loaded and matches current firmware, // otherwise reset storage. diff --git a/lib/pbio/sys/storage_data.h b/lib/pbio/sys/storage_data.h new file mode 100644 index 000000000..a51331656 --- /dev/null +++ b/lib/pbio/sys/storage_data.h @@ -0,0 +1,69 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) 2025 The Pybricks Authors + +// Data structures for non-volatile storage. + +// NB: Normally, pbsys code isn't referenced by drivers, however this is a +// special case where the block device driver needs to have the actual struct +// definition in order to ensure correct memory layout and alignment. + +#ifndef _PBSYS_SYS_STORAGE_DATA_H_ +#define _PBSYS_SYS_STORAGE_DATA_H_ + +#include + +#include +#include + +/** + * Information about one code slot. + * + * A size of 0 means that this slot is not used. The offset indicates where + * the program is stored in user storage. + * + * Code slots are *not* stored chronologically by slot id. Instead they are + * stored consecutively as they are received, with the newest program last. + * + * If a slot is already in use and a new program should be loaded into it, + * it is deleted by mem-moving any subsequent programs into its place, and + * appending the new program to be last again. Since a user is typically only + * iterating code in one slot, this is therefore usually the last stored + * program. This means memmoves happen very little, only when needed. + * + */ +typedef struct { + uint32_t offset; + uint32_t size; +} pbsys_storage_slot_info_t; + +/** + * Map of loaded data. All data types are little-endian. + */ +typedef struct { + /** + * End-user read-write accessible data. Everything after this is also + * user-readable but not writable. + */ + uint8_t user_data[PBSYS_CONFIG_STORAGE_USER_DATA_SIZE]; + /** + * First 8 symbols of the git hash of the firmware version used to create + * this data map. If this does not match the version of the running + * firmware, user data will be reset to 0. + */ + char stored_firmware_hash[8]; + /** + * System settings. Settings will be reset to defaults when the firmware + * version changes due to an update. + */ + pbsys_storage_settings_t settings; + /** + * Size and offset info for each slot. + */ + pbsys_storage_slot_info_t slot_info[PBSYS_CONFIG_STORAGE_NUM_SLOTS]; + /** + * Data of the application program (code + heap). + */ + uint8_t program_data[] __attribute__((aligned(sizeof(void *)))); +} pbsys_storage_data_map_t; + +#endif // _PBSYS_SYS_STORAGE_DATA_H_