rust-embedded / cortex-m

Low level access to Cortex-M processors
Apache License 2.0
835 stars 152 forks source link

Linker section alignment isn't suitable for MPU #524

Closed BigPeteB closed 2 months ago

BigPeteB commented 7 months ago

Correct me if I've gotten anything wrong—I'm not a general Cortex-M expert—but as I understand it, MPU regions must be 32-byte aligned. The linker script in cortex-m-rt uses 32-byte alignment only for .gnu.sgstubs to satisfy the SAU, but uses smaller alignment for all other sections.

The potential results could be very harmful, depending on how the MPU regions are configured. Consider the case of .text followed immediately by .rodata or .data:

  1. If we round towards lower addresses when configuring MPU regions, a few instructions of code would end up in the data region and be marked non-executable. This would be disastrous as it would crash.
  2. If we round towards higher addresses, a few bytes of data would end up in the code region and be marked executable. This could potentially be exploited if an attacker could find a way to manipulate that data so that it contains the instructions they desire (or they get lucky and the data already happens to correspond to instructions they can exploit) and then find a way to cause those instructions to be executed.

(I should mention, I'm considering cases that may be a bit beyond what cortex-m-rt supports out of the box, such as the FLASH memory region actually being in RAM, in which case it's not inherently read-only. But even if FLASH corresponds to read-only storage as cortex-m-rt expects, there are still potential crashes or security exploits waiting to happen.)

I think the linker script should be modified to use 32-byte alignment for regions that users would expect to configure separate MPU regions for. Given the current order of sections that are output, this affects almost all of them: 😭

Rearranging the linker sections could allow some regions to be consolidated and use fewer MPU regions, which would reduce the need to align/pad some of the sections. (Also, I just noticed that .gnu.sgstubs is placed after the comment /* ## Sections in RAM */, which is incorrect: it's placed in FLASH.)

adamgreig commented 7 months ago

MPU regions must be 32-byte aligned.

They must be aligned to the size of the region, which must be a power-of-two number of bytes and at least 32 bytes. So, the minimum alignment is 32 bytes, but in practice most sections will have much larger alignment requirements. I think this makes it impractical to adjust the default linker script to align everything; instead, someone wanting to split up the default sections like this will probably need to use a custom linker script with whatever specific alignments they need.

We do easily support custom linker scripts at least, but the documentation around that could be improved. I've made it clearer in #525.

jannic commented 2 months ago

@BigPeteB: Did that answer your question? If so, we could close the ticket.

BartMassey commented 2 months ago

I don't have permission to close this: perhaps someone who does could. It could always be reopened if more discussion is needed.