STMicroelectronics / STM32CubeL4

STM32Cube MCU Full Package for the STM32L4 series - (HAL + LL Drivers, CMSIS Core, CMSIS Device, MW libraries plus a set of Projects running on all boards provided by ST (Nucleo, Evaluation and Discovery Kits))
Other
262 stars 153 forks source link

Vector table definition in startup_*.s templates lacks alignment, may result in invalid offset in SCB->VTOR #22

Open apullin opened 3 years ago

apullin commented 3 years ago

Describe the set-up

This issue should apply to code built targeting any Cortex-M0/3/4 core when using the provided startup_stm32*.s templates to define the vector table and the use of the gcc toolchain.

This appears to be common to both ARMv6-M and ARMv7-M cores, as least per the ARM documentation.

v6-M : https://developer.arm.com/documentation/ddi0419/c/System-Level-Architecture/System-Address-Map/System-Control-Space--SCS-/Vector-Table-Offset-Register--VTOR?lang=en
V7-M: https://developer.arm.com/documentation/ddi0403/d/System-Level-Architecture/System-Address-Map/System-Control-Space--SCS-/Vector-Table-Offset-Register--VTOR?lang=en

In my case, I am encountering this issue on an STM32L496AG part on a custom-designed board.

arm-none-eabi-gcc (GNU Tools for Arm Embedded Processors 7-2017-q4-major) 7.2.1 20170904 (release) [ARM/embedded-7-branch revision 255204]

Describe the bug

There appears to be no alignment constraint on .isr_vector / g_pfnVectors, thus the table could end up being located to any address, e.g. 0x08010020 .

This could result in a vector table for which the offset cannot be loaded into SCB->VTOR.
As there is no checking for verification in SystemInit, the image-correct value of 0x08010020` might be loaded into SCB->VTOR, but due to the architecture constraints, the value of VTOR after write will be 0x08010000, thus pointing to an invalid view of the vector table.

This may also be an oversight on my part, but I believe I have run into it as an edge case.

How To Reproduce

Specific to my application (and this is where the mistake may be on my end):
I am building bootloadable images to be loaded into flash by a bootloader residing in the first 64K of flash. I am not using a "bank switching" technique; rather, the bootloader will erase main flash and write the contents of a new binary to a fixed address.

To build an image that will always load VTOR for the vector table provided in the image, In system_stem32L4xx.c I add: extern void* g_pfnVectors;
then, in SystemInit, I change:
SCB->VTOR = FLASH_BASE | VECT_TAB_OFFSET;
to
SCB->VTOR = (__IOM uint32_t)(&g_pfnVectors);

I am using a linker script that provides a MEMORY region with this configuration:

MEMORY
{
    RAM (xrw)      : ORIGIN = 0x20000000, LENGTH = 320K
    FLASH (rx)      : ORIGIN = 0x8010020, LENGTH = 942080
    DATA (rwx)      : ORIGIN = 0x080f6000, LENGTH = 40K
    QSPI (rx)      :   ORIGIN = 0x90000000, LENGTH = 8192K
}
  1. Build any example project using GCC, changing the FLASH start address in the linker script to e.g. 0x08010020

  2. Using a debugger, flash, and run with entry set to 0x08010020 to simulate entry via bootloader

  3. Break at SystemInit, run to the end of the function where SCB->VTOR is loaded

  4. Verify that g_pfnVectors is located to 0x08010020

  5. Observe that SCB->VTOR == 0x08010000 after write

  6. Continuing the program will likely result in a hardfault

Additional context

I believe the only remediation needed is to add an alignment constraint to the startup_stm*.s template file.

e.g., change

        .section    .isr_vector,"a",%progbits
    .type   g_pfnVectors, %object
    .size   g_pfnVectors, .-g_pfnVectors

g_pfnVectors:
    .word   _estack
        /* rest of the vector table ... */

to:

        .section    .isr_vector,"a",%progbits
    .type   g_pfnVectors, %object
    .size   g_pfnVectors, .-g_pfnVectors
    .align  7     /* require alignment to a value loadable into SCB->VTOR */

g_pfnVectors:
    .word   _estack
        /* rest of the vector table ... */

Although this is a change that may need to be made in every instance of the startup_stm*.s file, across all STM32Cube repos.

Beyond this fix, a more extensive solution may be to transition away from the asm startup file, and reimplement the template in C, where gcc alignment attributes could be applied.

FWIW, this issue arose in my usage of the bootloader provided in the aws/amazon-freertos repository. Their bootloader scheme relies on prepending any image with a 32B image "Image Descriptor", thus the 0x20 offset in my linker script.

RKOUSTM commented 3 years ago

Hi @apullin,

Thank you for this other report. You are right about this point. The issue has already been fixed internally. The fix below will be made available in the frame of a future release.

/** @addtogroup STM32L4xx_System_Private_Defines
  * @{
  */

+/* Note: Following vector table addresses must be defined in line with linker
+         configuration. */
+/*!< Uncomment the following line if you need to relocate the vector table
+    anywhere in Flash or Sram, else the vector table is kept at the automatic
+     remap of boot address selected */
+/* #define USER_VECT_TAB_ADDRESS */

+#if defined(USER_VECT_TAB_ADDRESS)
+/*!< Uncomment the following line if you need to relocate your vector Table
+     in Sram else user remap will be done in Flash. */
+/* #define VECT_TAB_SRAM */

+#if defined(VECT_TAB_SRAM)
+#define VECT_TAB_BASE_ADDRESS   SRAM1_BASE      /*!< Vector Table base address field.
+                                                     This value must be a multiple of 0x200. */
+#define VECT_TAB_OFFSET         0x00000000U     /*!< Vector Table base offset field.
+                                                     This value must be a multiple of 0x200. */
+#else
+#define VECT_TAB_BASE_ADDRESS   FLASH_BASE      /*!< Vector Table base address field.
                                                     This value must be a multiple of 0x200. */
+#define VECT_TAB_OFFSET         0x00000000U     /*!< Vector Table base offset field.
                                                     This value must be a multiple of 0x200. */
+#endif /* VECT_TAB_SRAM */
+#endif /* USER_VECT_TAB_ADDRESS */

In SystemInit() function, the following fix is proposed :

-  /* Configure the Vector Table location add offset address ------------------*/
-#ifdef VECT_TAB_SRAM
-  SCB->VTOR = SRAM_BASE | VECT_TAB_OFFSET; /* Vector Table Relocation in Internal SRAM */
-#else
-  SCB->VTOR = FLASH_BASE | VECT_TAB_OFFSET; /* Vector Table Relocation in Internal FLASH */
-#endif
+#if defined(USER_VECT_TAB_ADDRESS)
+  /* Configure the Vector Table location -------------------------------------*/
+  SCB->VTOR = VECT_TAB_BASE_ADDRESS | VECT_TAB_OFFSET;
+#endif

Thank you again for your contribution.

With regards,

apullin commented 3 years ago

My feedback here is (unless I am missing something): This would still allow someone to build an un-runnable binary.

Adding a _Static_assert might be a worthwhile addition, e.g.

#ifdef __GNUC__
_Static_assert( ((VECT_TAB_BASE_ADDRESS | VECT_TAB_OFFSET) % 0x200) == 0, "VTOR must be aligned to 0x200" )
#endif /* __GNUC__ */
RKOUSTM commented 3 years ago

Hi @apullin,

I hope you are fine. The issue you reported has been fixed in the frame of version v1.17.0 of the STM32CubeL4 published recently on GitHub. Thank you again for having reported. With regards,

ALABSTM commented 2 years ago

Hi @apullin,

Back to you about this point you reported. Indeed, it is not exactly related to the fix mentioned above. This issue is, hence, to be reopened and your report forwarded again to our development teams.

We will back to you as soon as we have their feedback. Thank you for your patience and thank you again for having reported.

With regards,

ALABSTM commented 2 years ago

ST Internal Reference: 117156