aarch64: Restrict address mappings in loader.rs#464
Conversation
Limit the 1:1 address mapping in the loader to cover only the necessary blocks using level-2 translation tables and with 2MB blocks. Don't map device memory at all, therefore, printf must not be used in the elfloader after the MMU is enabled. This prevents speculative accesses to device or secure memory which otherwise would cause faults or unwanted side-effects. Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
midnightveil
left a comment
midnightveil
left a comment
There was a problem hiding this comment.
Fine. Just some minor code cleanups to do.
| let (end_addr, _) = elf | ||
| .find_symbol("_bss_end") | ||
| .expect("Could not find 'end' symbol"); | ||
|
|
There was a problem hiding this comment.
I'd add a brief comment just saying what this is doing.
Specifically: it's mapping in just the loader itself; not all of RAM.
It might make sense to add extra linker script symbols for 'loader_start' and 'loader_end' rather than relying on text/bss end implicitly being at start/end.
Also text_addr -> start_addr
| .expect("Could not find 'end' symbol"); | ||
|
|
||
| if Aarch64::lvl1_index(text_addr) != Aarch64::lvl1_index(end_addr) { | ||
| eprintln!("ERROR: We only map 1GiB, but elfloader paddr range covers multiple GiB"); |
There was a problem hiding this comment.
I think we tend to use panic!("INTERNAL: <message>") here.
I'd also reword this: specifically the issue is the microkit loader (not elfloader) crosses a GiB boundary [or really that it requires multiple level 1 page tables].
It likely makes sense to do this change to both AArch64 and Riscv64, no sense it being different for either.
| if let Ok((uart_addr, _)) = elf.find_symbol("uart_addr") { | ||
| let data = elf | ||
| .get_data(uart_addr, 8) | ||
| .expect("uart_addr not initialized"); | ||
| let uart_base = u64::from_le_bytes(data[0..8].try_into().unwrap()); |
There was a problem hiding this comment.
(Not something you need to fix, just making this comment so I can link to it later):
It'd be nice if the kernel exposed the virtual address of where it wants its UART so we can map that in and get this feature working again.
| let pt_entry: u64 = (entry_idx as u64 + text_addr) | | ||
| (1 << 10) | // access flag | ||
| (3 << 8) | // inner sharable | ||
| (3 << 2) | // MT_NORMAL memory |
There was a problem hiding this comment.
alignment here and elsewhere is off
|
|
||
| let mut boot_lvl2_lower: [u8; PAGE_TABLE_SIZE] = [0; PAGE_TABLE_SIZE]; | ||
|
|
||
| let pt_entry = (boot_lvl2_lower_addr | 3).to_le_bytes(); |
There was a problem hiding this comment.
can we be consistent with the other examples and split out the meanings of the 3 flags into the individual bits?
2 participants
Hello,
This is a rework of the existing support committed in elfloader:
seL4/seL4_tools#244
and discussed here:
https://lists.sel4.systems/hyperkitty/list/devel@sel4.systems/thread/DJNDO5CUQBKGIA4SQHXCNQ3T6SKZEY4A/
This patch limits the 1:1 address mapping in the loader to cover only the necessary blocks using level-2 translation tables with 2MB blocks. The UART MMIO address is now exposed as a global uart_addr variable.
This prevents speculative accesses to device or secure memory, which could otherwise cause faults or unwanted side effects.
This has been tested only on STM32MP2. Testers on other platforms would be greatly appreciated.
Thank you for your review.