Hi Ard,
From: Ard Biesheuvel ardb@kernel.org Sent: mercredi 7 octobre 2020 12:27
On Tue, 6 Oct 2020 at 18:36, Patrick Delaunay patrick.delaunay@st.com wrote:
Hi,
On STM32MP15x platform we can use OP-TEE, loaded in DDR in a region protected by a firewall. This region is reserved in device with "no-map" property.
But sometime the platform boot failed in U-boot on a Cortex A7 access to this region (depending of the binary and the issue can change with compiler version or with code alignment), then the firewall raise a error, for example:
E/TC:0 tzc_it_handler:19 TZC permission failure E/TC:0 dump_fail_filter:420 Permission violation on filter 0 E/TC:0 dump_fail_filter:425 Violation @0xde5c6bf0, non-secure privileged
read,
AXI ID 5c0E/TC:0 Panic
After investigation, the forbidden access is a speculative request performed by the Cortex A7 because all the DDR is mapped as MEMORY with CACHEABLE property.
The issue is solved only when the region reserved by OP-TEE is no more mapped in U-Boot (mapped as DEVICE/NON-CACHEABLE wasn't enough) as it is already done in Linux kernel.
Spurious peculative accesses to device regions would be a severe silicon bug, so I wonder what is going on here.
(Apologies if we are rehashing stuff here that has already been discussed - I don't remember the details)
Are you sure that the speculative accesses were not caused by misconfigured CPU or page tables, missing TLB maintenance, etc etc? Because it really does smell like a software issue not a hardware issue.
I think that can be a general issue for ARM architecture: the no-map tag in device should be respected by U-boot, so I propose a generic solution in arm/lib/cache-cp15.c:dram_bank_mmu_setup().
This serie is composed by 7 patches
- 1..4/7: preliminary steps to support flags in library in lmb (as it is done in memblock.c in Linux)
- 5/7: unitary test on the added feature in lmb lib
- 6/7: save the no-map flags in lmb when the device tree is parsed
- 7/7: update the generic behavior for "no-map" region in arm/lib/cache-cp15.c::dram_bank_mmu_setup()
It is a rebase on master branch of previous RFC [2].
I can change this last patch if this feature is note required by other ARM architecture; it is a weak function so I can avoid to map the region with "no-map" property in device tree only for STM32MP architecture (in arch/arm/mach-stm32mp/cpu.c).
See also [1] which handle same speculative access on armv8 for area with Executable attribute.
[1] http://patchwork.ozlabs.org/project/uboot/patch/20200903000106.5016-1- marek.bykowski@gmail.com/ [2] http://patchwork.ozlabs.org/project/uboot/list/?series=199486&archive= both&state=*
Regards Patrick
Patrick Delaunay (7): lmb: Add support of flags for no-map properties lmb: add lmb_is_reserved_flags lmb: remove lmb_region.size lmb: add lmb_dump_region() function test: lmb: add test for lmb_reserve_flags image-fdt: save no-map parameter of reserve-memory arm: cache: cp15: don't map the reserved region with no-map property
arch/arm/include/asm/system.h | 3 + arch/arm/lib/cache-cp15.c | 19 ++++++- common/image-fdt.c | 23 +++++--- include/lmb.h | 22 +++++++- lib/lmb.c | 100 +++++++++++++++++++++++----------- test/lib/lmb.c | 89 ++++++++++++++++++++++++++++++ 6 files changed, 212 insertions(+), 44 deletions(-)
-- 2.17.1
No, I don't yet described the issue in details on mailing list. So I will explain the investigation done and my conclusion.
On STM32MP15x platform we have firewall to protect the access to DDR (TZC400 IP).
When OP-TEE is used, the boot chain is:
1/ ROM-Code (secure) 2/ TF-A (BL2) in SYSRAM (secure) 3/ OP-TEE in SYSRAM and DDR (secure) 4/ U-Boot in DDR 5/ Linux lernel
OP-TEE is loaded by TF-A BL2 - in SYRAM (for pager part) - in DDR (for pageable part).
U-Boot is loaded by TF-A BL2 in DDR.
When OP-TEE is execute, it protects with TZC400 firewall the reserved part of DDR as defined in device tree :
For example : ./arch/arm/dts/stm32mp157a-ed1-u-boot.dtsi:42: reserved-memory { optee@fe000000 { reg = <0xfe000000 0x02000000>; no-map; }; };
When OP-TEE is running in secure world (secure monitor is resident), it jump to normal world (unsecure) = U-Boot
After relocation, U-Boot maps all the DDR as normal memory (cacheable, bufferable, executable) and activate data cahe
Then, sometime (because depending of the compilation), the firewall detect an unsecure access to OP-TEE secured region when U-Boot is running.
We have investigate this access with gdb: - it is not an direct requet done by U-Boot code : we have no issue whe code is executed step by step - we have no issue when CACHE (instruction and data) is deactivated - no cache or TLB maintenance in this part of code - just after the PC, we found in memory a array whith content decoded as branch instruction to some address located in OP-TEE protected memory (even if the content of the array is not instruction but u32 values) and it is exactly this address which cause the firewall error.
PS: if I modify the code (by adding printf for example), the array change: it content is no more see as branch instruction and the issue disappears.
My conclusion: the A7 core "see" the branch instruction near the PC with address in DDR and this address is marked as cacheable/executable in MMU So the instruction cache load this code in the cache instruction (preloaded for Cortex A7 pipeline).
I found this note in ARM documentation (found in armv8 but it is the same for armV7):
https://developer.arm.com/architectures/learn-the-architecture/armv8-a-memor...
Note: There is a subtle distinction here that is easy to miss. Marking a region as Device prevents speculative data accesses only. Marking a region as non-executable prevents speculative instruction accesses. This means that, to prevent any speculative accesses, a region must be marked as both Device and non-executable.
For me to avoid any unexpected access to protected memory by firewall the OP-TEE reserved memory must at least be configured as device memory and not executable...
But after tests, it wasn't enough. Even if we set the OP-TEE memory with DCACHE_OFF value, TZC400 see other issue for secure to no secure transition access (during SMC execution for request to secure monitor).
Then I remember a other requirement in ARM architecture: to avoid unexpected behavior the same region should be mapped not mapped as device and memory: (OP-TEE mark the reserved region as normal memory / cacheable only accessible by secure world / protected by TZC400)
Reference = ARMv7 Architecture reference:
A3.5.7 Memory access restrictions
Behavior is UNPREDICTABLE if the same memory location: — is marked as Shareable Normal and Non-shareable Normal — is marked as having different memory types (Normal, Device, or Strongly-ordered) — is marked as having different cacheability attributes — is marked as being Shareable Device and Non-shareable Device memory. Such memory marking contradictions can occur, for example, by the use of aliases in a virtual to physical address mapping
It is why I propose this patchset to un-map the OP-TEE memory, as it is indicated in device tree; I don't see other solution to respect the ARM requirements.
So for me it is not a SOC issue or a SW bug, but an ARM architecture constraint for speculative access to normal memory region (marked cacheable/shareable/executable).
This prevent any issue for STM32MP15x platform but also for other platforms when some part of memory must be protected (no access allowed/protected by firewall) BEFORE U-Boot execution.
I hope that it is more clear now.
Regards
Patrick