knurling-rs / flip-link

Adds zero-cost stack overflow protection to your embedded programs
Apache License 2.0
279 stars 8 forks source link

Does not work with `alloc-cortex-m` #62

Open laptou opened 2 years ago

laptou commented 2 years ago

When I try to use alloc-cortex-m in the following fashion:

#![no_main]
#![no_std]
#![feature(alloc_error_handler)]

extern crate alloc;

use alloc_cortex_m::CortexMHeap;
use core::alloc::Layout;
use defmt::*;

#[global_allocator]
static ALLOCATOR: CortexMHeap = CortexMHeap::empty();

#[cortex_m_rt::entry]
fn main() -> ! {
    defmt::timestamp!("{=usize}", now());

    info!("initializing heap");

    let start = cortex_m_rt::heap_start() as usize;
    let size = 4;
    debug!("start = {:#010x}, size = {:#010x}", start, size);

    unsafe { ALLOCATOR.init(start, size) }

   // other code
}

Then the program panics immediately upon trying to initialize the heap, and the heap start address appears to be wrong (I am using the STM32F411, so SRAM begins at 0x2000 0000 and ends at 0x2002 0000).

    Finished dev [unoptimized + debuginfo] target(s) in 3.78s
     Running `probe-run --chip STM32F411VETx target/thumbv7em-none-eabihf/debug/tasks`
(HOST) INFO  flashing program (49 pages / 49.00 KiB)
(HOST) INFO  success!
────────────────────────────────────────────────────────────────────────────────
0 INFO  initializing heap
└─ tasks::__cortex_m_rt_main::_ @ src/bin/tasks.rs:28
0 DEBUG start = 0x2001fffc, size = 0x00000004
└─ tasks::__cortex_m_rt_main::_ @ src/bin/tasks.rs:33
────────────────────────────────────────────────────────────────────────────────
stack backtrace:
   0: HardFaultTrampoline
      <exception entry>
   1: linked_list_allocator::hole::HoleList::new
        at /home/ibiyemi/.cargo/registry/src/github.com-1ecc6299db9ec823/linked_list_allocator-0.8.11/src/hole.rs:50:9
   2: linked_list_allocator::Heap::init
        at /home/ibiyemi/.cargo/registry/src/github.com-1ecc6299db9ec823/linked_list_allocator-0.8.11/src/lib.rs:73:22
   3: alloc_cortex_m::CortexMHeap::init::{{closure}}
        at /home/ibiyemi/.cargo/registry/src/github.com-1ecc6299db9ec823/alloc-cortex-m-0.4.1/src/lib.rs:58:13
   4: cortex_m::interrupt::free
        at /home/ibiyemi/.cargo/registry/src/github.com-1ecc6299db9ec823/cortex-m-0.7.3/src/interrupt.rs:64:13
   5: alloc_cortex_m::CortexMHeap::init
        at /home/ibiyemi/.cargo/registry/src/github.com-1ecc6299db9ec823/alloc-cortex-m-0.4.1/src/lib.rs:57:9
   6: tasks::__cortex_m_rt_main
        at src/bin/tasks.rs:35:14
   7: main
        at src/bin/tasks.rs:24:1
   8: Reset
(HOST) WARN  call stack was corrupted; unwinding could not be completed
(HOST) ERROR the program panicked

If I disable flip-link by commenting out "-C", "linker=flip-link" in .cargo/config.toml, then this panic does not happen, and a more reasonable start address is printed:

    Finished dev [unoptimized + debuginfo] target(s) in 52.83s
     Running `probe-run --chip STM32F411VETx target/thumbv7em-none-eabihf/debug/tasks`
(HOST) INFO  flashing program (49 pages / 49.00 KiB)
(HOST) INFO  success!
────────────────────────────────────────────────────────────────────────────────
0 INFO  initializing heap
└─ tasks::__cortex_m_rt_main::_ @ src/bin/tasks.rs:28
0 DEBUG start = 0x20000454, size = 0x00010000
└─ tasks::__cortex_m_rt_main::_ @ src/bin/tasks.rs:33

It has occurred to me that the point of flip-link is to allow stack overflow errors to be caught by relying on the stack crashing into the RAM boundary. Does this mean that flip-link is incompatible with the use of a heap, or does the heap need to be placed above the stack in memory? If it's the latter, how would I achieve that?

jannic commented 2 years ago

alloc-cortex-m will create a heap starting at the location __sheap. With a conventional memory layout, there is an empty space above __sheap:

+-------------+ <-- stack_start
|    stack    |
|      |      |
|      v      |
| ~~~~~~~~~~~ |
|             |
+-------------+ <-- __sheap
| .bss+.data  |
+-------------+

However, this space is shared with the stack (which grows downward from the end of the memory region.) Therefore, a growing stack may overwrite the heap (and .bss, .data). This is exactly the issue solved by flip-link, by re-arranging the memory:

+-------------+ <-- __sheap
| .bss+.data  |
+-------------+ <-- stack_start
|    stack    |
|      |      |
|      v      |
| ~~~~~~~~~~~ |
|             |
+-------------+

But as you can see, flip-link doesn't leave space for a heap above __sheap, so alloc-cortex-m can't place the heap there.

So, currenty, you are right: flip-link is not compatible with alloc-cortex-m.

As far as I can tell, it would not be difficult to reserve some space for the heap. It may be as easy as adding some configurable value to used_ram_length at https://github.com/knurling-rs/flip-link/blob/main/src/main.rs#L72.

jonathanpallant commented 2 years ago

Will flip-link work correctly if you place your heap in a dedicated section and tell alloc-cortex-m to use that section?

The whole premise of flip-link is to ensure that stack overflows cause a hard fault and are easy to detect, but the whole premise of alloc-cortex-m seems to be that stack and heap grow towards each other. I think it is these concepts that are incompatible.

jannic commented 2 years ago

I only skimmed over the source code of flip-link and alloc-cortex-m, so I may be missing something. From what I understood, it may actually work:

flip-link just takes the lowest and the highest allocated address, and assumes that the difference is the amount of RAM needed by .bss+.data. Then it rewrites the linker script and sets the total amount of RAM to that value - and moves the start of the RAM region as far up as possible. As a result, __sheap points to the end of the RAM region (or a few bytes before that, depending on alignment.)

alloc-cortex-m just uses that __sheap symbol to place the heap. The size of the heap is specified manually by the developer.

So, if flip-link could be configured to place the RAM region to a sufficiently lower address, there would be space for the heap to be used by alloc-cortex-m.

The stack will still grow downwards, and the heap upwards. I think this would be compatible with alloc-cortex-m. At least I don't see any code in alloc-cortex-m which implicitly assumes that the heap grows towards the stack.

Of course, the heap size configured by the call to CortexMHeap::init(start, size) must fit into the space reserved for the heap.

But that's not much different from the behavior without flip-link: Instead of memory corruption by the collision of heap+stack, one would get a HardFault because the heap would reach the end of RAM. It's still the responsibility of the developer to set the heap size appropriately.

Additionally, flip-link could add a symbol pointing to the end of the heap, and alloc-cortex-m could use that to automatically set the heap size. But such a mechanism is not yet implemented in alloc-cortex-m.

jonathanpallant commented 2 years ago

To crib your picture:

+-------------+ - top of RAM
|             |
| ~~~~~~~~~~~ |
|      ^      |
|      |      |
|    heap     |
+-------------+ <-- __sheap
| .bss+.data  |
+-------------+ <-- stack_start
|    stack    |
|      |      |
|      v      |
| ~~~~~~~~~~~ |
|             |
+-------------+ - bottom of RAM
jannic commented 2 years ago

Exactly. One difference between stack and heap not shown in that picture: While the stack grows at runtime, and can actually reach the bottom, heap size (at least with alloc-cortex-m) is defined statically and a full heap will cause an allocation failure, not an exception. (In theory, an allocator could implement some kind of brk() call actually growing the heap. But I don't see how that could be useful on a microcontroller without an MMU)

jonathanpallant commented 2 years ago

Of course. I was thinking of newly which uses the sbrk sys call to increase the heap size.

Sympatron commented 2 years ago

__sheap is defined in cortex-m-rt and it's usefulness relies on the fact that it points to the end of the stack and can therefore be used as a start for an arbitrarily large heap as long as the stack doesn't grow to much:

+-------------+ <-- stack_start
|    stack    |
|      |      |
|      v      |
| ~~~~~~~~~~~ |
|             |
+-------------+ <-- __sheap + heap size
|             |
|    heap     |
+-------------+ <-- __sheap
| .bss+.data  |
+-------------+ - bottom of RAM

The easiest solution is probably to have a static mut HEAP: [u8; N] (where N is the desired heap size) and then initialize alloc-cortex-m with unsafe { ALLOCATOR.init(HEAP as *u8 as usize, HEAP.len()) } That way your heap will end up in bss and not cause problems with flip-link.

jannic commented 2 years ago

@Sympatron, in principle, that should work. However, be careful with that static mut HEAP: It will overlap the memory handed out by the allocator. That way, I'd be easy to get multiple mutable references to the same memory location, causing undefined behavior.

laptou commented 2 years ago

We might be able to fix that using a OnceCell or something...

jonathanpallant commented 2 years ago

You could put it in the block that inits the allocator. You do still technically have two references in flight at the same time, but it's a very short window.

{
    static mut HEAP: [u8; NNN] = [0; NNN];
    allocator.init(&mut HEAP, ...);
}
Sympatron commented 2 years ago

@jannic That's why ALLOCATOR.init() is unsafe. By calling it, you have to promise not to use it again. I would argue that this solution is still way safer than the implicit promise you make to the standard linker that your stack won't grow into your heap.

If you want to be extra safe you can use something like what @jonathanpallant suggested or encapsulate it in a separate crate to make it impossible to reference directly.

OnceCell only guarantees to let you write to it once, so I think this wouldn't really help. You would need something that lets you only read once. cortex_m::singleton! would be the safest option for cortex-m at least IMO.