search

riscv-software-src / opensbi

RISC-V Open Source Supervisor Binary Interface
Other
1.03k stars 511 forks source link

Too small fw_heap_size makes OpenSBI corrupt memory #335

Open eenurkka opened 11 months ago

eenurkka commented 11 months ago

Heap minimum appears to be 16k. This is quite a chunk. This is not documented anywhere nor checked anywhere.

Otherwise sbi_heap.c fails:

hpctrl.hksize = hpctrl.size / HEAP_HOUSEKEEPING_FACTOR; hpctrl.hksize &= ~((unsigned long)HEAP_BASE_ALIGN - 1);

8k: (0x2000 / 16) & ~(1024 - 1) = 0 (Fail!) 16k: (0x4000 / 16) & ~(1024 - 1) = 0x400 (Ok)

For example, with fw_heap_size = 8k, hpctrl.hksize gets to be zero making the OpenSBI corrupt memory wildly. Everything goes bust and boom in that case.

mateoconlechuga commented 11 months ago

Personally I feel that the firmware linker script should have sections for the stack, heap, and scratch whose sizes are controlled through menuconfig for the platform. What are the thoughts on this? Doing this would mean these checks removed since it's all controlled through link time.

Currently the way it is implemented it is difficult if not impossible to determine what memory OpenSBI is actually using since it is just expecting memory to exist beyond the end of the program. This seems... unsafe?

avpatel commented 11 months ago

All three per-CPU stack, per-CPU scratch, and system wide heap are scaled at boot time based on the number of CPUs. This allows us to use the same FW binary on multiple platforms. We can further improve this by allowing platforms to override heap size using a DT property but this is not available yet.

mateoconlechuga commented 11 months ago

This allows us to use the same FW binary on multiple platforms

The same firmware binary doesn't work on multiple platforms though since the platform is responsible for allocating the heap size based on the expected number of CPUs (like the template here just shows a magic "1" value here representing 1 hart: https://github.com/riscv-software-src/opensbi/blob/master/platform/template/platform.c#L155).

The number of CPUs and the per-hart stack size could be passed to the linker script - right now the developer of the platform has to know the number of CPUs anyway since it's a platform-specific define at compile time here and here.