aarch64: Restrict address mappings in loader.rs#464
Conversation
midnightveil
left a comment
midnightveil
left a comment
There was a problem hiding this comment.
Fine. Just some minor code cleanups to do.
tool/microkit/src/loader.rs
Outdated
| .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.
There was a problem hiding this comment.
I am not familiar with the Riscv64 MMU tables, and I do not have access to a testing platform. Is there any volunteer to do the adaptation?
There was a problem hiding this comment.
That's fair. We're planning on changing a few things after these are good to go, anyway, and having to retest.
There was a problem hiding this comment.
thank you. I noticed the CI doesn’t build for riscv64 without uart_addr. I’ll guard it to fix the build before your changes.
tool/microkit/src/loader.rs
Outdated
| 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 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?
There was a problem hiding this comment.
This this is a 2 bit marker for the 1GB L1 table entry here, not individual bits
There was a problem hiding this comment.
Ah, right, we do |3 in a bunch of places below. I'll change that at a later point.
There was a problem hiding this comment.
Thanks, better to handle it globally yes. I’ll add a comment since “3” means different things depending on the granule (page or table)
bruelc
force-pushed
the
master-mmuloader
branch
3 times, most recently
from
0b1d9ca to
12dd82c
Compare
tool/microkit/src/loader.rs
Outdated
| let pt_entry: u64 = (entry_idx as u64 + start_addr) | | ||
| (1 << 10) | // Access flag | ||
| (3 << 8) | // Sharable | ||
| (3 << 2) | // MT_NORMAL memory |
There was a problem hiding this comment.
Looking at this more. Did you intentionally make this 3 << 2? That would correspond to MT_NORMAL_NC, not MT_NORMAL.
There was a problem hiding this comment.
Argh no! That’s an early debugging trace… thanks for noticing.
There was a problem hiding this comment.
Fixed: Note that I also had to update #465 to flush the cache before jumping to the secondary core
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>
bruelc
force-pushed
the
master-mmuloader
branch
from
9dbc0d7 to
b62c440
Compare
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.