Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Too small fw_heap_size makes OpenSBI corrupt memory #335

Open
eenurkka opened this issue Nov 27, 2023 · 3 comments
Open

Too small fw_heap_size makes OpenSBI corrupt memory #335

eenurkka opened this issue Nov 27, 2023 · 3 comments

Comments

@eenurkka
Copy link

eenurkka commented Nov 27, 2023

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
Copy link
Contributor

mateoconlechuga commented Dec 8, 2023

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
Copy link
Collaborator

avpatel commented Dec 9, 2023

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
Copy link
Contributor

mateoconlechuga commented Dec 9, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants